Re: [PATCH v2 14/19] tree-diff: rework diff_tree interface to be sha1 based
Kirill Smelkov k...@mns.spb.ru writes:
The downside is that try_to_follow_renames(), if active, we cause
re-reading of 2 initial trees, which was negligible based on my timings,
That would depend on how often the codepath triggered in your test
case, but is totally understandable. It fires only when the path we
have been following disappears from the parent, and the processing
of try-to-follow itself is very compute-intensive (it needs to run
find-copies-harder logic) that will end up reading many subtrees of
the two initial trees; two more reading of tree objects will be
dwarfed by the actual processing.
and which is outweighed cogently by the upsides.
Changes since v1:
- don't need to touch diff.h, as diff_tree() became static.
Nice. I wonder if it is an option to let the function keep its name
diff_tree() without renaming it to __diff_tree_whatever(), though.
tree-diff.c | 60
1 file changed, 28 insertions(+), 32 deletions(-)
diff --git a/tree-diff.c b/tree-diff.c
index b99622c..f90acf5 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -137,12 +137,17 @@ static void skip_uninteresting(struct tree_desc *t,
struct strbuf *base,
}
}
-static int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
- const char *base_str, struct diff_options *opt)
+static int __diff_tree_sha1(const unsigned char *old, const unsigned char
*new,
+ const char *base_str, struct diff_options *opt)
{
+ struct tree_desc t1, t2;
+ void *t1tree, *t2tree;
struct strbuf base;
int baselen = strlen(base_str);
+ t1tree = fill_tree_descriptor(t1, old);
+ t2tree = fill_tree_descriptor(t2, new);
+
/* Enable recursion indefinitely */
opt-pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
@@ -155,39 +160,41 @@ static int diff_tree(struct tree_desc *t1, struct
tree_desc *t2,
if (diff_can_quit_early(opt))
break;
if (opt-pathspec.nr) {
- skip_uninteresting(t1, base, opt);
- skip_uninteresting(t2, base, opt);
+ skip_uninteresting(t1, base, opt);
+ skip_uninteresting(t2, base, opt);
}
- if (!t1-size !t2-size)
+ if (!t1.size !t2.size)
break;
- cmp = tree_entry_pathcmp(t1, t2);
+ cmp = tree_entry_pathcmp(t1, t2);
/* t1 = t2 */
if (cmp == 0) {
if (DIFF_OPT_TST(opt, FIND_COPIES_HARDER) ||
- hashcmp(t1-entry.sha1, t2-entry.sha1) ||
- (t1-entry.mode != t2-entry.mode))
- show_path(base, opt, t1, t2);
+ hashcmp(t1.entry.sha1, t2.entry.sha1) ||
+ (t1.entry.mode != t2.entry.mode))
+ show_path(base, opt, t1, t2);
- update_tree_entry(t1);
- update_tree_entry(t2);
+ update_tree_entry(t1);
+ update_tree_entry(t2);
}
/* t1 t2 */
else if (cmp 0) {
- show_path(base, opt, t1, /*t2=*/NULL);
- update_tree_entry(t1);
+ show_path(base, opt, t1, /*t2=*/NULL);
+ update_tree_entry(t1);
}
/* t1 t2 */
else {
- show_path(base, opt, /*t1=*/NULL, t2);
- update_tree_entry(t2);
+ show_path(base, opt, /*t1=*/NULL, t2);
+ update_tree_entry(t2);
}
}
strbuf_release(base);
+ free(t2tree);
+ free(t1tree);
return 0;
}
@@ -202,7 +209,7 @@ static inline int diff_might_be_rename(void)
!DIFF_FILE_VALID(diff_queued_diff.queue[0]-one);
}
-static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc
*t2, const char *base, struct diff_options *opt)
+static void try_to_follow_renames(const unsigned char *old, const unsigned
char *new, const char *base, struct diff_options *opt)
{
struct diff_options diff_opts;
struct diff_queue_struct *q = diff_queued_diff;
@@ -240,7 +247,7 @@ static void try_to_follow_renames(struct tree_desc *t1,
struct tree_desc *t2, co
diff_opts.break_opt = opt-break_opt;
diff_opts.rename_score = opt-rename_score;
diff_setup_done(diff_opts);
- diff_tree(t1, t2, base, diff_opts);
+ __diff_tree_sha1(old, new, base, diff_opts);
diffcore_std(diff_opts);
free_pathspec(diff_opts.pathspec);
@@ -301,23 +308,12 @@ static void try_to_follow_renames(struct tree_desc *t1,
struct tree_desc *t2, co
int diff_tree_sha1(const unsigned char
Re: [PATCH v2 16/19] tree-diff: reuse base str(buf) memory on sub-tree recursion
Kirill Smelkov k...@mns.spb.ru writes:
instead of allocating it all the time for every subtree in
__diff_tree_sha1, let's allocate it once in diff_tree_sha1, and then all
callee just use it in stacking style, without memory allocations.
This should be faster, and for me this change gives the following
slight speedups for
git log --raw --no-abbrev --no-renames --format='%H'
navy.gitlinux.git v3.10..v3.11
before 0.618s 1.903s
after 0.611s 1.889s
speedup 1.1%0.7%
Signed-off-by: Kirill Smelkov k...@mns.spb.ru
---
Changes since v1:
- don't need to touch diff.h, as the function we are changing became static.
tree-diff.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/tree-diff.c b/tree-diff.c
index aea0297..c76821d 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -115,7 +115,7 @@ static void show_path(struct strbuf *base, struct
diff_options *opt,
if (recurse) {
strbuf_addch(base, '/');
__diff_tree_sha1(t1 ? t1-entry.sha1 : NULL,
- t2 ? t2-entry.sha1 : NULL, base-buf, opt);
+ t2 ? t2-entry.sha1 : NULL, base, opt);
}
strbuf_setlen(base, old_baselen);
I was scratching my head for a while, after seeing that there does
not seem to be any *new* code added by this patch in order to
store-away the original length and restore the singleton base buffer
to the original length after using addch/addstr to extend it.
But I see that the code has already been prepared to do this
conversion. I wonder why we didn't do this earlier ;-)
Looks good. Thanks.
@@ -138,12 +138,10 @@ static void skip_uninteresting(struct tree_desc *t,
struct strbuf *base,
}
static int __diff_tree_sha1(const unsigned char *old, const unsigned char
*new,
- const char *base_str, struct diff_options *opt)
+ struct strbuf *base, struct diff_options *opt)
{
struct tree_desc t1, t2;
void *t1tree, *t2tree;
- struct strbuf base;
- int baselen = strlen(base_str);
t1tree = fill_tree_descriptor(t1, old);
t2tree = fill_tree_descriptor(t2, new);
@@ -151,17 +149,14 @@ static int __diff_tree_sha1(const unsigned char *old,
const unsigned char *new,
/* Enable recursion indefinitely */
opt-pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
- strbuf_init(base, PATH_MAX);
- strbuf_add(base, base_str, baselen);
-
for (;;) {
int cmp;
if (diff_can_quit_early(opt))
break;
if (opt-pathspec.nr) {
- skip_uninteresting(t1, base, opt);
- skip_uninteresting(t2, base, opt);
+ skip_uninteresting(t1, base, opt);
+ skip_uninteresting(t2, base, opt);
}
if (!t1.size !t2.size)
break;
@@ -173,7 +168,7 @@ static int __diff_tree_sha1(const unsigned char *old,
const unsigned char *new,
if (DIFF_OPT_TST(opt, FIND_COPIES_HARDER) ||
hashcmp(t1.entry.sha1, t2.entry.sha1) ||
(t1.entry.mode != t2.entry.mode))
- show_path(base, opt, t1, t2);
+ show_path(base, opt, t1, t2);
update_tree_entry(t1);
update_tree_entry(t2);
@@ -181,18 +176,17 @@ static int __diff_tree_sha1(const unsigned char *old,
const unsigned char *new,
/* t1 t2 */
else if (cmp 0) {
- show_path(base, opt, t1, /*t2=*/NULL);
+ show_path(base, opt, t1, /*t2=*/NULL);
update_tree_entry(t1);
}
/* t1 t2 */
else {
- show_path(base, opt, /*t1=*/NULL, t2);
+ show_path(base, opt, /*t1=*/NULL, t2);
update_tree_entry(t2);
}
}
- strbuf_release(base);
free(t2tree);
free(t1tree);
return 0;
@@ -209,7 +203,7 @@ static inline int diff_might_be_rename(void)
!DIFF_FILE_VALID(diff_queued_diff.queue[0]-one);
}
-static void try_to_follow_renames(const unsigned char *old, const unsigned
char *new, const char *base, struct diff_options *opt)
+static void try_to_follow_renames(const unsigned char *old, const unsigned
char *new, struct strbuf *base, struct diff_options *opt)
{
struct diff_options diff_opts;
struct diff_queue_struct *q = diff_queued_diff;
@@ -306,13 +300,19 @@ static void try_to_follow_renames(const unsigned char
*old, const unsigned char
q-nr = 1;
}
-int diff_tree_sha1(const unsigned char *old, const unsigned char *new,
Re: [PATCH 17/19] Portable alloca for Git
Kirill Smelkov k...@mns.spb.ru writes: On Fri, Feb 28, 2014 at 06:19:58PM +0100, Erik Faye-Lund wrote: On Fri, Feb 28, 2014 at 6:00 PM, Kirill Smelkov k...@mns.spb.ru wrote: ... In fact that would be maybe preferred, for maintainers to enable alloca with knowledge and testing, as one person can't have them all at hand. Yeah, you're probably right. Erik, the patch has been merged into pu today. Would you please follow-up with tested MINGW change? Sooo I lost track but this discussion seems to have petered out around here. I think the copy we have had for a while on 'pu' is basically sound, and can easily built on by platform folks by adding or removing the -DHAVE_ALLOCA_H from the Makefile. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls
Jeff King p...@peff.net writes: I do not have a problem with that, as it implicitly covers all of the tests following it. I do not think it is particularly necessary, though. Assuming we start with a known test environment and avoiding polluting it for further tests are basic principles of _all_ test scripts. They should be, but I suspect majority of tests, especially the older ones, do have dependencies on earlier test pieces X-. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/10] t4209: factor out helper function test_log_icase()
René Scharfe l@web.de writes:
Am 24.03.2014 22:10, schrieb Jeff King:
On Mon, Mar 24, 2014 at 11:22:30AM -0700, Junio C Hamano wrote:
+test_log_icase() {
+ test_log $@ --regexp-ignore-case
+ test_log $@ -i
-cascade broken? Will squash in an obvious fix.
I don't think so. This is happening outside of test_expect_success,
which is run by test_log. So adding a means that if the first test
fails, we do not bother to run the second one at all, which is not what
we want.
Right; this function runs two independent tests and is left out
intentionally.
Ahh, sorry and 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: Git push race condition?
Matthieu Moy matthieu@grenoble-inp.fr writes: What you describe really looks like a force-push, or a hook doing a ref update (e.g. a hook on a dev branch that updates master if the code passes tests or so). ... or a filesystem that is broken. But I thought this is just a plain-vanilla ext4, nothing exotic, so Puzzled. -- To unsubscribe from this list: send the line 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] builtin/apply.c: use iswspace() to detect line-ending-like chars
Michael Haggerty mhag...@alum.mit.edu writes: -while ((*last1 == '\r') || (*last1 == '\n')) +while (iswspace(*last1)) last1--; -while ((*last2 == '\r') || (*last2 == '\n')) +while (iswspace(*last2)) last2--; /* skip leading whitespace */ In addition to Eric's comments... What happens if the string consists *only* of whitespace? Also, why would casting char to wchar_t without any conversion be safe and/or sane? I would sort-of understand if the change were to use isspace(), but I do not think that is a correct conversion, either. Isn't a pair of strings a bc and a bc supposed not to match? My understanding is that two strings that differ only at places where they have runs of whitespaces whose length differ are to compare the same, e.g. a_bc__ and a__bc_ (SP replaced with _ to make them stand out). Ignoring whitespace change is very different from ignoring all whitespaces (the latter of which would make a b and ab match). As a tangent, I have a suspicion that the current implementation may be wrong at the beginning of the string. Wouldn't it match abc and abc, even though these two strings shouldn't match? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls
Jeff King p...@peff.net writes: Hmph. I am looking at git show HEAD^:t/t0001-init.sh after applying this patch, and it does look consistently done with GIT_CONFIG and GIT_DIR (I am not sure about GIT_WORK_TREE but from a cursory read it is done consistently for tests on non-bare repositories). I don't understand why we stop bothering with the unsets starting with init with --template. Are those variables not important to the outcome of that and later tests, or did the author simply not bother because they are noops? If I had to guess (without running blame, so it may well turn out that the author may turn out to be me ;-), it is simply the author not being careful. -- To unsubscribe from this list: send the line 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: [RFC/PATCH] Better control of the tests run by a test suite
Jeff King p...@peff.net writes:
On Mon, Mar 24, 2014 at 01:49:44AM -0700, Ilya Bobyr wrote:
Here are some examples of how functionality added by the patch
could be used. In order to run setup tests and then only a
specific test (use case 1) one can do:
$ ./t-init.sh --run='1 2 25'
or:
$ ./t-init.sh --run='3 25'
('=' is also supported, as well as '' and '=').
I don't have anything against this in principle, but I suspect it will
end up being a big pain to figure out which of the early tests are
required to set up the state, and which are not. Having makes
specifying it easier, but you still have to read the test script to
figure out which tests need to be run.
Likewise.
I wonder if it would make sense to auto-select tests that match a
regex like set.?up|create? A while ago, Jonathan made a claim that
this would cover most tests that are dependencies of other tests. I did
not believe him, but looking into it, I recall that we did seem to have
quite a few matching that pattern. If there were a good feature like
this that gave us a reason to follow that pattern, I think people might
fix the remainder
This may be worth experimenting with, 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] Allow --pretty to be passed to git-describe.
Cyril Roelandt tipec...@gmail.com writes: In some cases, ony may want to find the the most recent tag that is reachable from a commit and have it pretty printed, using the formatting options available in git-log and git-show. Sorry, but I do not understand the motivation I can read from these three lines well enough to say that such a change makes any sense. It somewhat sounds similar to adding a --long option to git show so that git show --long v1.9.0 will act as if it were running a git log v1.9.0. Yes, you can add any option to tell a command to do something that is usually considered to be a task for some other command, but does it even make sense to do so in the first place? -- To unsubscribe from this list: send the line 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 000/144] Use the $( ... ) construct for command substitution instead of using the back-quotes
Elia Pinto gitter.spi...@gmail.com writes: The patch is simple but involves a large number of files with different authors. Being simple I think it is wasteful to cc a large number of different people for doing a review. We'd somehow need a way to parallelize the reviews, though. Being simple is never an indication that the series is less likely to contain bugs, and it often is an opposite. It is easier to slip bugs in to a simple, repetitive and long 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: What's cooking in git.git (Mar 2014, #05; Mon, 24)
Michael Haggerty mhag...@alum.mit.edu writes: On 03/24/2014 09:27 PM, Junio C Hamano wrote: [...] * an/branch-config-message (2014-03-24) 1 commit - branch.c: install_branch_config: simplify if chain Will merge to 'next'. The Signed-off-by line in this commit shows the name as only Adam. Adam, unless this is your full name, please add a S-o-b line with your full name as per Documentation/SubmittingPatches. This is important to help us track the provenance of all code in Git. Michael Thanks for bringing this up. But for this particular change, I think there is very little that could become problematic on the provenance; there are only 34 sane ways to switch on two independent boolean variables to choose one out of four actions, and they are all similar and obvious. -- To unsubscribe from this list: send the line 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: Borrowing objects from nearby repositories
Ævar Arnfjörð Bjarmason ava...@gmail.com writes: 1) Introduce '--borrow' to `git-fetch`. This would behave similarly to '--reference', except that it operates on a temporary basis, and does not assume that the reference repository will exist after the operation completes, so any used objects are copied into the local objects database. In theory, this mechanism would be distinct from --reference', so if both are used, some objects would be copied, and some objects would be accessible via a reference repository referenced by the alternates file. Isn't this the same as git clone --reference path --no-hardlinks url ? Also without --no-hardlinks we're not assuming that the other repo doesn't go away (you could rm-rf it), just that the files won't be *modified*, which Git won't do, but you could manually do with other tools, so the default is to hardlink. I think that the standard practice with the existing toolset is to clone with reference and then repack. That is: $ git clone --reference borrowee git://over/there mine $ cd mine $ git repack -a -d And then you can try this: $ mv .git/objects/info/alternates .git/objects/info/alternates.disabled $ git fsck to make sure that you are no longer borrowing anything from the borrowee. Once you are satisfied, you can remove the saved-away alternates.disabled file. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] Clarify pre-push hook documentation
David Cowden dco...@gmail.com writes: The documentation as-is does not mention that the pre-push hook is executed even when there is nothing to push. This can lead a new reader to believe there will always be lines fed to the script's standard input and cause minor confusion as to what is happening when there are no lines provided to the pre-push script. Signed-off-by: David Cowden dco...@gmail.com --- Notes: I'm not sure if I've covered every case here. If there are more cases to consider, please let me know and I can update to include them. I do not think of any offhand, but a more important point that I was trying to get at was that we should not give an incorrect impression to the readers that the scenario that is described is the only case they need to be worried about by pretending to be exhaustive. The may in your wording This may happen when may be good enough to hint that these may not be the only cases. c.f. http://stackoverflow.com/questions/22585091/git-hooks-pre-push-script-does-not-receive-input-via-stdin Documentation/githooks.txt | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index d954bf6..1fd6da9 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -203,6 +203,15 @@ SHA-1` will be 40 `0`. If the local commit was specified by something other than a name which could be expanded (such as `HEAD~`, or a SHA-1) it will be supplied as it was originally given. +The hook is executed regardless of whether changes will actually be pushed or +not. This may happen when 'git push' is called and: + + - the remote ref is already up to date, or + - pushing to the remote ref cannot be handled by a simple fast-forward + +In other words, the script is called for every push. In the event that nothing +is to be pushed, no data will be provided on the script's standard input. When two things are to be pushed, the script will see the two things. When one thing is to be pushed, the script will see the one thing. When no thing is to be pushed, the script will see no thing on its standard input. But isn't that obvious? I still wonder if we really need to single out that nothing case. The more important thing is that it is invoked even in the 0-thing pushed case, and the list of things pushed that is given to the hook happens to be empty is an obvious natural fallout. If this hook exits with a non-zero status, 'git push' will abort without pushing anything. Information about why the push is rejected may be sent to the user by writing to standard error. -- To unsubscribe from this list: send the line 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] test-lib: Document short options in t/README
Ilya Bobyr ilya.bo...@gmail.com writes: On 3/24/2014 4:39 AM, Ramsay Jones wrote: On 24/03/14 08:49, Ilya Bobyr wrote: Most arguments that could be provided to a test have short forms. Unless documented the only way to learn then is to read the code. Signed-off-by: Ilya Bobyr ilya.bo...@gmail.com --- t/README | 10 +- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/t/README b/t/README index caeeb9d..ccb5989 100644 --- a/t/README +++ b/t/README @@ -71,7 +71,7 @@ You can pass --verbose (or -v), --debug (or -d), and --immediate (or -i) command line argument to the test, or by setting GIT_TEST_OPTS appropriately before running make. ---verbose:: +-v,--verbose:: OK [...] ---valgrind=tool:: +-v,--valgrind=tool:: The -v short option is taken, above ... :-P Right %) Thanks :) This one starts only with --va, will fix it. Please don't. In general, when option names can be shortened by taking a unique prefix, it is better not to give short form in the documentation at all. There is no guarantee that the short form you happen to pick when you document it will continue to be unique forever. When we add another --vasomething option, --va will become ambiguous and one of these two things must happen: (1) --valgrind and --vasomething are equally useful and often used. Neither will get --va and either --val or --vas needs to be given. (2) Because we documented --va as --valgrind, people feel that they are entitled to expect --va will stay forever to be a shorthand for --valgrind and nothing else. The shortened forms will be between --va (or longer prefix of --valgrind) and --vas (or longer prefix of --vasomething). We would rather want to see (1), as people new to the system do not have to learn that --valgrind can be spelled --va merely by being the first to appear, and --vasomething must be spelled --vas because it happened to come later. Longer term, nobody should care how the system evolved into the current shape, but (2) will require that to understand and remember why one is --va and the other has to be --vas. We already have this suboptimal (2) situation between --valgrind and --verbose options, but a shorter form v that is used for verbose is so widely understood and used that I think it is an acceptable exception. So --verbose:: +-v:: Give verbose output from the test is OK, but --valgrind can be shortened to --va is not. -- To unsubscribe from this list: send the line 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 12/19] tree-diff: remove special-case diff-emitting code for empty-tree cases
Kirill Smelkov k...@navytux.spb.ru writes:
static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)
{
struct name_entry *e1, *e2;
int cmp;
+ if (!t1-size)
+ return t2-size ? +1 /* +∞ c */ : 0 /* +∞ = +∞ */;
+ else if (!t2-size)
+ return -1; /* c +∞ */
Where do these c come from? I somehow feel that these comments
are making it harder to understand what is going on.
c means some finite constant here. When I was studying at school and
at the university, it was common to denote constants via this letter -
i.e. in algebra and operators they often show scalar multiplication as
c·A (or α·A)
etc. I understand it could maybe be confusing (but it came to me as
surprise), so would the following be maybe better:
if (!t1-size)
return t2-size ? +1 /* +∞ const */ : 0 /* +∞ = +∞ */;
else if (!t2-size)
return -1; /* const +∞ */
?
Not better at all, I am afraid. A const in the code usually means
something that does not change, as opposed to a variable, but what
you are saying here is t1 does not have an element but t2 still
does. Pretend as if t1 has a virtual/fake element that is larger
than any real element t2 may happen to have at the head of its
queue, and you are labeling that real element at the head of t2
as const, but as the walker advances, the head element in t1 and
t2 will change---they are not const in that sense, and the reader
is left scratching his head seeing const there, wondering what the
author of the comment meant.
real or concrete might be better a phrasing, but I do not think
having /* +inf concrete */ there helps the reader understand
what is going on in the first place. Perhaps:
/*
* When one side is empty, pretend that it has an element
* that sorts later than what the other non-empty side has,
* so that the caller advances the non-empty side without
* touching the empty side.
*/
if (!t1-size)
return !t2-size ? 0 : 1;
else if (!t2-size)
return -1;
or something?
--
To unsubscribe from this list: send the line 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 14/19] tree-diff: rework diff_tree interface to be sha1 based
Kirill Smelkov k...@navytux.spb.ru writes: What are the downsides of __ prefix by the way? Aren't these names reserved for compiler/runtime implementations? -- To unsubscribe from this list: send the line 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 000/142] Use the $( ... ) construct for command substitution instead of using the back-quotes
Elia Pinto gitter.spi...@gmail.com writes: This is a second reroll after the Matthieu Moy review. Changes from v1: - Dropped the silly patches to t6111-rev-list-treesame.sh, t0204-gettext-reencode-sanity.sh. - Simple reformatting of the commit message. - added the toy script used for doing the patch. In other words, Mattheiu pointed out two examples of breakages and they were the only mistakes after you re-reviewed the mechanical changes, and there is no more? - Cleaning up is a good thing, but - Mechanical and mindless conversion is always error-prone, and - Eyeballing each and every change is required, but - Nobody has time or energy to go through 140+ patches in one go, with enough concentration necessary to do so without making mistakes (this applies to yourself, too---producing mechanical replacement is a no-cost thing, finding mistakes in mechanical replacement takes real effort). That is why we strongly discourage people from such a whole-tree clean-up just for the sake of clean-up and nothing else, and instead encourage them to clean-up as a preparatory step for real work on the files involved. Sure, it would not give us as wide a coverage as a mechanical whole-tree replacement in one-go, but that is the only practical way to avoid mistakes. If it were four patches per every Monday kind of trickle, we might be able to spend some review bandwidth while reviewing other patches, though. So I am not very enthused about reviewing and applying these patches in their current form. 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 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
Elia Pinto gitter.spi...@gmail.com writes:
The Git CodingGuidelines prefer the $( ... ) construct for command
substitution instead of using the back-quotes, or grave accents (`..`).
The backquoted form is the historical method for command substitution,
and is supported by POSIX. However, all but the simplest uses become
complicated quickly. In particular, embedded command substitutions
and/or the use of double quotes require careful escaping with the backslash
character. Because of this the POSIX shell adopted the $(…) feature from
the Korn shell.
The patch was generated by the simple script
for _f in $(find . -name *.sh)
do
sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done
and then carefully proofread is sorely needed here.
What is that non-breaking space doing at the beginning of an
indented line, or is it just my environment, by the way?
Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
check-builtins.sh |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/check-builtins.sh b/check-builtins.sh
index d6fe6cf..07cff69 100755
--- a/check-builtins.sh
+++ b/check-builtins.sh
@@ -14,8 +14,8 @@ sort |
bad=0
while read builtin
do
- base=`expr $builtin : 'git-\(.*\)'`
- x=`sed -ne 's/.*{ '$base', \(cmd_[^, ]*\).*/'$base' \1/p' git.c`
+ base=$(expr $builtin : 'git-\(.*\)')
+ x=$(sed -ne 's/.*{ '$base', \(cmd_[^, ]*\).*/'$base' \1/p' git.c)
if test -z $x
then
echo $base is builtin but not listed in git.c command list
Looks ok to me.
--
To unsubscribe from this list: send the line 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 009/142] t0001-init.sh: use the $( ... ) construct for command substitution
This conflicts with a topic in flight. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Mar 2014, #06; Tue, 25)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. More topics merged to 'master', many of which are fallouts from GSoC microprojects. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to master] * bb/diff-no-index-dotdot (2014-03-19) 2 commits (merged to 'next' on 2014-03-20 at 352f48c) + diff-no-index: replace manual ./.. check with is_dot_or_dotdot() + diff-no-index: rename read_directory() * cp/am-patch-format-doc (2014-03-17) 2 commits (merged to 'next' on 2014-03-17 at 7437c77) + Documentation/git-am: typofix (merged to 'next' on 2014-03-12 at 17c3ada) + Documentation/git-am: Document supported --patch-format options * dm/configure-iconv-locale-charset (2014-03-11) 1 commit (merged to 'next' on 2014-03-20 at 4443bfd) + configure.ac: link with -liconv for locale_charset() * jk/lib-terminal-lazy (2014-03-14) 1 commit (merged to 'next' on 2014-03-20 at 5de832f) + t/lib-terminal: make TTY a lazy prerequisite The test helper lib-terminal always run an actual test_expect_* when included, which screwed up with the use of skil-all that may have to be done later. * jk/mv-submodules-fix (2014-03-17) 2 commits (merged to 'next' on 2014-03-17 at 7cae3b1) + mv: prevent mismatched data when ignoring errors. + builtin/mv: fix out of bounds write git mv that moves a submodule forgot to adjust the array that uses to keep track of which submodules were to be moved to update its configuration. * jk/warn-on-object-refname-ambiguity (2014-03-13) 4 commits (merged to 'next' on 2014-03-17 at 3f8e98e) + rev-list: disable object/refname ambiguity check with --stdin + cat-file: restore warn_on_object_refname_ambiguity flag + cat-file: fix a minor memory leak in batch_objects + cat-file: refactor error handling of batch_objects * mh/remove-subtree-long-pathname-fix (2014-03-13) 2 commits (merged to 'next' on 2014-03-17 at 68cc994) + entry.c: fix possible buffer overflow in remove_subtree() + checkout_entry(): use the strbuf throughout the function Length limit for the pathname used when removing a path in a deep subdirectory has been removed to avoid buffer overflows. * nd/commit-editor-cleanup (2014-02-25) 3 commits (merged to 'next' on 2014-03-17 at 986605d) + commit: add --cleanup=scissors + wt-status.c: move cut-line print code out to wt_status_add_cut_line + wt-status.c: make cut_line[] const to shrink .data section a bit git commit --cleanup=mode learned a new mode, scissors. * nd/indent-fix-connect-c (2014-03-13) 1 commit (merged to 'next' on 2014-03-17 at a109efc) + connect.c: SP after }, not TAB * nd/index-pack-error-message (2014-03-17) 1 commit (merged to 'next' on 2014-03-20 at 4d722ac) + index-pack: report error using the correct variable git index-pack used a wrong variable to name the keep-file in an error message when the file cannot be written or closed. * rr/doc-merge-strategies (2014-03-17) 1 commit (merged to 'next' on 2014-03-20 at d31f415) + Documentation/merge-strategies: avoid hyphenated commands There were a few instances of 'git-foo' remaining in the documentation that should have been spelled 'git foo'. * ss/test-on-mingw-rsync-path-no-absolute (2014-03-19) 1 commit (merged to 'next' on 2014-03-20 at 2b7b95d) + t5510: Do not use $(pwd) when fetching / pushing / pulling via rsync * us/printf-not-echo (2014-03-18) 2 commits (merged to 'next' on 2014-03-20 at 41205c8) + test-lib.sh: do not echo caller-supplied strings + rebase -i: do not echo random user-supplied strings rebase -i produced a broken insn sheet when the title of a commit happened to contain '\n' (or ended with '\c') due to a careless use of 'echo'. -- [New Topics] * jk/tests-cleanup (2014-03-21) 12 commits - t0001: drop subshells just for cd - t0001: drop useless subshells - t0001: use test_must_fail - t0001: use test_config_global - t0001: use test_path_is_* - t0001: make symlink reinit test more careful - t: prefer git config --file to GIT_CONFIG - t: prefer git config --file to GIT_CONFIG with test_must_fail - t: stop using GIT_CONFIG to cross repo boundaries - t: drop useless sane_unset GIT_* calls - t/test-lib: drop redundant unset of GIT_CONFIG - t/Makefile: stop setting GIT_CONFIG (this branch uses dt/tests-with-env-not-subshell.) Will merge to 'next'. -- [Stalled] * tr/merge-recursive-index-only (2014-02-05) 3 commits - merge-recursive: -Xindex-only to leave worktree unchanged - merge-recursive: internal flag to avoid touching the worktree - merge-recursive: remove dead conditional in update_stages() (this branch is used by
Re: [PATCH v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
Junio C Hamano gits...@pobox.com writes:
Elia Pinto gitter.spi...@gmail.com writes:
The Git CodingGuidelines prefer the $( ... ) construct for command
substitution instead of using the back-quotes, or grave accents (`..`).
The backquoted form is the historical method for command substitution,
and is supported by POSIX. However, all but the simplest uses become
complicated quickly. In particular, embedded command substitutions
and/or the use of double quotes require careful escaping with the backslash
character. Because of this the POSIX shell adopted the $(…) feature from
the Korn shell.
The patch was generated by the simple script
for _f in $(find . -name *.sh)
do
sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done
and then carefully proofread is sorely needed here.
What is that non-breaking space doing at the beginning of an
indented line, or is it just my environment, by the way?
I've reworded the above like so:
check-builtins.sh: use the $(...) construct for command substitution
The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes, or grave accents
(`...`).
The backquoted form is the historical method for command
substitution, and is supported by POSIX. However, all but the
simplest uses become complicated quickly. In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.
The patch was generated by:
for _f in $(find . -name *.sh)
do
sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done
and then carefully proof-read.
Signed-off-by: Elia Pinto gitter.spi...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
I looked at [*1*] to pick between back-quote and backquote; this
is a related tangent, but in that page, you will find this gem:
Within the backquoted style of command substitution, backslash
shall retain its literal meaning, except when followed by: '$',
'`', or backslash.
Stated another way, `` = $() conversion will make backslash inside
to bahave differently, and we would need to be careful when doing
such a change.
I've looked at and queued 001 and 002.
[Reference]
*1*
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_03
--
To unsubscribe from this list: send the line 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 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
Junio C Hamano gits...@pobox.com writes: I've reworded the above like so: check-builtins.sh: use the $(...) construct for command substitution The Git CodingGuidelines prefer the $(...) construct for command substitution instead of using the backquotes, or grave accents (`...`). ... but I think the , or grave accents should go. -- To unsubscribe from this list: send the line 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: [RFC/PATCH 1/4] test-lib: add test_dir_is_empty()
Jens Lehmann jens.lehm...@web.de writes:
For the upcoming submodule test framework we often need to assert that an
empty directory exists in the work tree. Add the test_dir_is_empty()
function which asserts that the given argument is an empty directory.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
I believe this one is pretty straightforward (unless I missed that this
functionality already exists someplace I forgot to look ;-).
I am not very thrilled to see that it depends on . and .. to
always exist, which may be true for all POSIX filesystems, but
still...
Do expected callsites of this helper care if $1 is a symbolic link
that points at an empty directory?
What do expected callsites really want to ensure? In other words,
why do they care if the directory is empty? Is it to make sure,
after some operation, they can rmdir the directory?
t/test-lib-functions.sh | 11 +++
1 file changed, 11 insertions(+)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 158e10a..93d10cd 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -489,6 +489,17 @@ test_path_is_dir () {
fi
}
+# Check if the directory exists and is empty as expected, barf otherwise.
+test_dir_is_empty () {
+ test_path_is_dir $1
+ if test $(ls -a1 $1 | wc -l) != 2
+ then
+ echo Directory '$1' is not empty, it contains:
+ ls -la $1
+ return 1
+ fi
+}
+
test_path_is_missing () {
if [ -e $1 ]
then
--
To unsubscribe from this list: send the line 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 12/19] tree-diff: remove special-case diff-emitting code for empty-tree cases
Kirill Smelkov k...@navytux.spb.ru writes: On Mon, Mar 24, 2014 at 02:18:10PM -0700, Junio C Hamano wrote: Kirill Smelkov k...@mns.spb.ru writes: via teaching tree_entry_pathcmp() how to compare empty tree descriptors: Drop this line, as you explain the pretend empty compares bigger than anything else idea later anyway? This early part of the proposed log message made me hiccup while reading it. Hmm, I was trying to show the big picture first and only then details... The subject should be sufficient for the big picture. OK, we are removing the special casing is what we expect the reader to get. Then, this While walking trees, we iterate their entries from lowest to highest in sort order, so empty tree means all entries were already went over. sets the background. OK, the code walks two trees, both have sorted elements, in parallel. is what we want the reader to understand. Then the next part gives the idea of pretending that the empty-side always compare later than the non-empty side while doing that parallel walking (similar to merge). So, yes, I think it is a good presentation order to give big picture punch-line first on the subject, some background and then the solution. -- To unsubscribe from this list: send the line 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: Borrowing objects from nearby repositories
Junio C Hamano gits...@pobox.com writes: Ævar Arnfjörð Bjarmason ava...@gmail.com writes: 1) Introduce '--borrow' to `git-fetch`. This would behave similarly to '--reference', except that it operates on a temporary basis, and does not assume that the reference repository will exist after the operation completes, so any used objects are copied into the local objects database. In theory, this mechanism would be distinct from --reference', so if both are used, some objects would be copied, and some objects would be accessible via a reference repository referenced by the alternates file. Isn't this the same as git clone --reference path --no-hardlinks url ? Also without --no-hardlinks we're not assuming that the other repo doesn't go away (you could rm-rf it), just that the files won't be *modified*, which Git won't do, but you could manually do with other tools, so the default is to hardlink. I think that the standard practice with the existing toolset is to clone with reference and then repack. That is: $ git clone --reference borrowee git://over/there mine $ cd mine $ git repack -a -d And then you can try this: $ mv .git/objects/info/alternates .git/objects/info/alternates.disabled $ git fsck to make sure that you are no longer borrowing anything from the borrowee. Once you are satisfied, you can remove the saved-away alternates.disabled file. Oh, I forgot to say that I am not opposed if somebody wants to teach git clone a new option to copy its objects from two places, (hopefully) the majority from near-by reference repository and the remainder over the network, without permanently relying on the former via the alternates mechanism. The implementation of such a feature could even literally be clone with reference first and then repack at least initially but even in the final version. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] log: add --show-linear-break to help see non-linear history
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: 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 pclo...@gmail.com --- On Fri, Mar 21, 2014 at 2:15 AM, Junio C Hamano gits...@pobox.com wrote: * Get rid of saved_linear, use another flag in struct object instead I cannot offhand say if I like this change or not. A flag bit is a scarce and limited resource; commit slabs felt more suited for implementation of corner case eye-candies. I leave it in bit 26. We can move it out when we run low on flag bits. * Fix not showing the break bar after a root commit if the dag graph has multiple roots I definitely do not like the way a commit-list data structure is abused to hold a phoney element that points at a NULL with its item pointer. Allocate a single bit in revs that says I haven't done anything yet if you want to catch the first-ness without breaking what commit_list_insert() and friends are expecting to see---they never expect to see a NULL asked to be on the list, AFAIK. Fixed. * Make it work with --graph (although I don't really see the point of using both at the same time) I do not see the point, either. I vaguely recall that the previous iteration refused the combination at the option parser level, which I think would be the right thing to do. Fixed. All changes look good to me. Especially on the last one because the new printf() calls do not even attempt to call into the graph API to tell it that it created a gutter above or give it a chance to draw all vertical lines to connect the graph part. Will replace and advance them to Will merge to 'next' state. 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: Borrowing objects from nearby repositories
Andrew Keller and...@kellerfarm.com writes: On Mar 25, 2014, at 6:17 PM, Junio C Hamano gits...@pobox.com wrote: ... I think that the standard practice with the existing toolset is to clone with reference and then repack. That is: $ git clone --reference borrowee git://over/there mine $ cd mine $ git repack -a -d And then you can try this: $ mv .git/objects/info/alternates .git/objects/info/alternates.disabled $ git fsck to make sure that you are no longer borrowing anything from the borrowee. Once you are satisfied, you can remove the saved-away alternates.disabled file. Oh, I forgot to say that I am not opposed if somebody wants to teach git clone a new option to copy its objects from two places, (hopefully) the majority from near-by reference repository and the remainder over the network, without permanently relying on the former via the alternates mechanism. The implementation of such a feature could even literally be clone with reference first and then repack at least initially but even in the final version. [Administrivia: please wrap your lines to a reasonable length] That was actually one of my first ideas - adding some sort of '--auto-repack' option to git-clone. It's a relatively small change, and would work. However, keeping in mind my end goal of automating the feature to the point where you could run simply 'git clone url', an '--auto-repack' option is more difficult to undo. You would need a new parameter to disable the automatic adding of reference repositories, and a new parameter to undo '--auto-repack', and you'd have to remember to actually undo both of those settings. In contrast, if the new feature was '--borrow', and the evolution of the feature was a global configuration 'fetch.autoBorrow', then to turn it off temporarily, one only needs a single new parameter '--no-auto-borrow'. I think this is a cleaner approach than the former, although much more work. I think you may have misread me. With the new option, I was hinting that the clone --reference repack rm alternates will be an acceptable internal implementation of the --borrow option that was mentioned in the thread. I am not sure where you got the auto-repack from. One of the reasons you may have misread me may be because I made it sound as if this may work and when it works you will be happy, but if it does not work you did not lose very much by mentioning mv fsck. That wasn't what I meant. The repack -a procedure is to make the borrower repository no longer dependent on the borrowee, and it is supposed to always work. In fact, this behaviour was the whole reason why repack later learned its -l option to disable it, because people who cloned with --reference in order to reduce the disk footprint by sharing older and more common objects [*1*] were rightfully surprised to see that the borrowed objects were copied over to their borrower repository when they ran repack [*2*]. Because this is clone, there is nothing complex to undo. Either it succeeds, or you remove the whole new directory if anything fails. I said even in the final version for a simple reason: you cannot cannot do realistically any better than the clone --reference repack -a d rm alternates sequence. But you would need to know a few things about how Git works in order to come to that realisation. Here are some: * clone --borrow (or whatever we end up calling the option) must talk to two repositories: - We will need to have one upload-pack session with the distant origin repository over the network, which will send a complete pack. - We need to also copy objects that weren't sent from the distant origin to our repository from the reference one. * A single repack -a -d (without -l) after clone --reference is already a way to do exactly what you need---enumerate what are missing in the packfile that was received from the distant origin and come up with packfile(s) that contain all and only objects the cloned repository needs. * You cannot easily concatenate multiple packfiles into a single one (or append runs of objects to an existing packfile) to come up with a single packfile. You _could_ shoehorn the logic to enumerate and read from the reference, and append them at the end of the packfile received from the distant origin repository into the part that talks to the distant origin repository, but the object layout in the resulting packfile will be suboptimal [*3*] and the code complexity required to do so is not worth it [*4*]. [Footnotes] *1* From the point of view of supporting both camps, i.e. those who want their borrower repositories to keep sharing the objects with the borrowee repository and those who want to use a borrowee repository temporarily while cloning only to reduce the network cost from the distant upstream, the current option name --reference and the proposed name --borrow are backwards
Re: [PATCH/RFC 0/6] reuse deltas found by bitmaps
Jeff King p...@peff.net writes: 2. When considering whether a delta can be reused, check the bitmaps to see if the client has the base. If so, allow reuse. ... The implementation I'm including here is the one I've shown before, which does (2). Part of the reason that I'm reposting it before looking further into these options is that I've written a t/perf script that can help with the analysis. Conceptually, that feels like a natural extension for the thin pack transfer. I wouldn't foresee a correctness issue, as long as the fetcher runs index-pack --fix-thin at their end. -- To unsubscribe from this list: send the line 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] builtin/apply.c: use iswspace() to detect line-ending-like chars
George Papanikolaou g3orge@gmail.com writes: On Tue, Mar 25, 2014 at 6:54 AM, Junio C Hamano gits...@pobox.com wrote: As a tangent, I have a suspicion that the current implementation may be wrong at the beginning of the string. Wouldn't it match abc and abc, even though these two strings shouldn't match? Wouldn't that be accomplished by just removing the leading whitespace check? Yes. I was wondering *what* semantics we want in the first place; how to implement what I suggested is so trivial that it goes without saying for the intended audiences of that comment ;-). I'm somewhat confused about what the function should match. I haven't grasped it. This function is used when attempting to resurrect a patch that is whitespace-damaged. The patch may want to change a line a_bc in the original into something else [*1*], and we may not find a_bc in the current source, but there may be a__bc (two spaces instead of one the whitespace-damaged patch claims to expect). By ignoring the amount of whitespaces, it forces git apply to consider that a_bc in the broken patch meant to refer to a__bc in reality. I _think_ the original motivation of ignore_ws_change was to match the --ignore-space-change option of diff, i.e. ignore changes in the amount of white space. I just checked the source (xdiff/xutils.c) and made sure that git diff does not treat the beginning of line any differently hence _a_bc and a_bc are not considered a match under its --ignore-space-change option. The current implementation of apply --ignore-space-change that ignores leading whitespaces (not ignore changes in the amount of leading whitespaces) is likely to be a bug from this point of view. But I wanted to hear opinions from other Git experts [*2*]. Hence my As a tangent, I have a suspicion. [Footnote] *1* This mode is not enabled by default. I am not even sure if anybody sane would (or should) use this option. Sure, the fuzzy match may be able to find the original line that the patch author may meant to patch even when it is whiltespace-damaged because it does not fully trust what the original lines exactly say (i.e. context lines prefixed by and old lines prefixed by -). What makes it sane for us to trust what the replacement lines (i.e. new lines prefixed by +) in such a mangled patch says? *2* For example, somebody may be able to point out that this is meant to match the option of the same name 'diff' has, which is my assumption that leads to the above discussion, may not be true. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] log: do not segfault on gmtime errors
Jeff King p...@peff.net writes: On Wed, Mar 26, 2014 at 11:05:59AM +, Charles Bailey wrote: On Mon, Feb 24, 2014 at 02:49:05AM -0500, Jeff King wrote: +# date is within 2^63-1, but enough to choke glibc's gmtime +test_expect_success 'absurdly far-in-future dates produce sentinel' ' + commit=$(munge_author_date HEAD 99) + echo Thu Jan 1 00:00:00 1970 + expect + git log -1 --format=%ad $commit actual + test_cmp expect actual +' Git on AIX seems happy to convert this to Thu Oct 24 18:46:39 162396404 -0700. I don't know if this is correct for the given input but the test fails. Ick. I am not sure that dates at that range are even meaningful (will October really exist in the year 162396404? Did they account for all the leap-seconds between then and now?). But at the same time, I cannot fault their gmtime for just extrapolating the current rules out as far as we ask them to. Unlike the FreeBSD thing that René brought up, this is not a problem in the code, but just in the test. So I think our options are basically: 1. Scrap the test as unportable. 2. Hard-code a few expected values. I'd be unsurprised if some other system comes up with a slightly different date in 162396404, so we may end up needing several of these. I think I'd lean towards (2), just because it is testing an actual feature of the code, and I'd like to continue doing so. And while we may end up with a handful of values, there's probably not _that_ many independent implementations of gmtime in the wild. Or 3. Just make sure that 'git log' does not segfault? -- To unsubscribe from this list: send the line 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] t4212: handle systems with post-apocalyptic gmtime
Jeff King p...@peff.net writes:
+cmp_one_of () {
+ for candidate in $@; do
Style ;-)
+ echo $candidate expect
+ test_cmp expect actual
+ return 0
+ done
+ return 1
+}
It actually may be easier to understand if you write a trivial case
statement at the sole calling site of this helper function, though.
In any case, would it really be essential to make sure that the
output shows a phony (or a seemingly real) datestamp for this test?
The primary thing you wanted to achieve by the gmtime gave us NULL,
let's substitute it with an arbitrary value to avoid dereferencing
the NULL change was *not* that we see that same arbitrary value
comes out of the system, but that we do not die by attempting to
reference the NULL, I think. Not dying is the primary thing we want
to (and we already do) test, no?
+# date is within 2^63-1, but enough to choke glibc's gmtime.
+# We check that either the date broke gmtime (and we return the
+# usual epoch value), or gmtime gave us some sensible value.
+#
+# The sensible values are determined experimentally. The first
+# is from AIX.
+test_expect_success 'absurdly far-in-future dates' '
commit=$(munge_author_date HEAD 99)
- echo Thu Jan 1 00:00:00 1970 + expect
git log -1 --format=%ad $commit actual
- test_cmp expect actual
+ cmp_one_of \
+ Thu Jan 1 00:00:00 1970 + \
+ Thu Oct 24 18:46:39 162396404 -0700
'
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 5/5] log: do not segfault on gmtime errors
Jeff King p...@peff.net writes: On Wed, Mar 26, 2014 at 11:58:49AM -0700, Junio C Hamano wrote: Unlike the FreeBSD thing that René brought up, this is not a problem in the code, but just in the test. So I think our options are basically: 1. Scrap the test as unportable. 2. Hard-code a few expected values. I'd be unsurprised if some other system comes up with a slightly different date in 162396404, so we may end up needing several of these. I think I'd lean towards (2), just because it is testing an actual feature of the code, and I'd like to continue doing so. And while we may end up with a handful of values, there's probably not _that_ many independent implementations of gmtime in the wild. Or 3. Just make sure that 'git log' does not segfault? That would not test the FreeBSD case, which does not segfault, but returns a bogus sentinel value. I don't know how important that is. This is such a minor feature that it is not worth a lot of maintenance headache in the test. But I also do not know if this is going to be the last report, or we will have a bunch of other systems that need their own special values put into the test. I didn't quite realize that your objective of the change was to signal the failure of gmtime for all implementations, i.e. both (1) the ones that signal an error by giving NULL back to us and (2) the ones that fail to signal an error but leave bogus result (FreeBSD, but it appears AIX might be also giving us another bogus value), by using a single sane sentinel value. If we can do that consistently on every platform, that would indeed be great. But if that is the case, we should not have to maintain special case values in the test code, I would think. The test instead should expect the output to have that single sentinel value, and if the workaround code fails to detect a breakage in the platform gmtime(), the special case logic to catch these platform-specific breakages should go that timestamp that cannot be grokked by gmtime---replace it with a sentinel logic, no? -- To unsubscribe from this list: send the line 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 18/25] setup.c: support multi-checkout repo setup
Duy Nguyen pclo...@gmail.com writes: On Tue, Mar 25, 2014 at 08:52:13PM +0700, Duy Nguyen wrote: On Mon, Mar 24, 2014 at 9:52 PM, Torsten Bögershausen tbo...@web.de wrote: Did I report that t1501 fails when there is a softlink in $PWD ? /home/tb/projects is a softlink to /disc5/projects/ Yes you did and I forgot. I have fixed it, running test suite and will send the reroll soon. Junio, it seems you have picked up all minor changes after v5. Resending the whole series for one fix seems overkill. Could you just --autosquash this one in? Gladly; thanks for a quick turnaround. -- 8 -- Subject: [PATCH] fixup! setup.c: support multi-checkout repo setup --- t/t1501-worktree.sh | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh index 2ac4424..e6ac7a4 100755 --- a/t/t1501-worktree.sh +++ b/t/t1501-worktree.sh @@ -359,7 +359,7 @@ test_expect_success 'GIT_DIR set (1)' ' ( cd work GIT_DIR=../gitfile git rev-parse --git-common-dir actual - echo $TRASH_DIRECTORY/repo.git expect + test-path-utils real_path $TRASH_DIRECTORY/repo.git expect test_cmp expect actual ) ' @@ -370,7 +370,7 @@ test_expect_success 'GIT_DIR set (2)' ' ( cd work GIT_DIR=../gitfile git rev-parse --git-common-dir actual - echo $TRASH_DIRECTORY/repo.git expect + test-path-utils real_path $TRASH_DIRECTORY/repo.git expect test_cmp expect actual ) ' @@ -381,7 +381,7 @@ test_expect_success 'Auto discovery' ' ( cd work git rev-parse --git-common-dir actual - echo $TRASH_DIRECTORY/repo.git expect + test-path-utils real_path $TRASH_DIRECTORY/repo.git expect test_cmp expect actual echo haha data1 git add data1 @@ -399,7 +399,7 @@ test_expect_success '$GIT_DIR/common overrides core.worktree' ' ( cd work git rev-parse --git-common-dir actual - echo $TRASH_DIRECTORY/repo.git expect + test-path-utils real_path $TRASH_DIRECTORY/repo.git expect test_cmp expect actual echo haha data2 git add data2 -- To unsubscribe from this list: send the line 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] apply --ignore-space-change: lines with and without leading whitespaces do not match
The fuzzy_matchlines() function is used when attempting to resurrect
a patch that is whitespace-damaged, or when applying a patch that
was produced against an old codebase to the codebase after
indentation change.
The patch may want to change a line a_bc (_ is used throught
this description for a whitespace to make it stand out) in the
original into something else, and we may not find a_bc in the
current source, but there may be a__bc (two spaces instead of one
the whitespace-damaged patch claims to expect). By ignoring the
amount of whitespaces, it forces git apply to consider that a_bc
in the broken patch meant to refer to a__bc in reality.
However, the implementation special cases a run of whitespaces at
the beginning of a line and makes abc match _abc, even though a
whitespace in the middle of string never matches a 0-width gap,
e.g. a_bc does not match abc. A run of whitespace at the end of
one string does not match a 0-width end of line on the other line,
either, e.g. abc_ does not match abc.
Fix this inconsistency by making the code skip leading whitespaces
only when both strings begin with a whitespace. This makes the
option mean the same as the option of the same name in diff and
git diff.
Note that I am not sure if anybody sane should use this option in
the first place. The fuzzy match logic may be able to find the
original line that the patch author may have meant to touch because
it does not fully trust what the original lines say (i.e. context
lines prefixed by and old lines prefixed by - does not have to
exactly match the contents the patch is applied to). There is no
reason for us to trust what the replacement lines (i.e. new lines
prefixed by +) say, either, but with this option enabled, we end
up copying these new lines with suspicious whitespace distributions
literally into the patched result. But as long as we keep it, we
should make it do its insane thing consistently.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/apply.c| 12 +++-
t/t4107-apply-ignore-whitespace.sh | 12
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 0189523..8b9d3de 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -300,11 +300,13 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
while ((*last2 == '\r') || (*last2 == '\n'))
last2--;
- /* skip leading whitespace */
- while (isspace(*s1) (s1 = last1))
- s1++;
- while (isspace(*s2) (s2 = last2))
- s2++;
+ /* skip leading whitespaces, if both begin with whitespace */
+ if (s1 = last1 s2 = last2 isspace(*s1) isspace(*s2)) {
+ 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;
diff --git a/t/t4107-apply-ignore-whitespace.sh
b/t/t4107-apply-ignore-whitespace.sh
index b04fc8f..9e29b52 100755
--- a/t/t4107-apply-ignore-whitespace.sh
+++ b/t/t4107-apply-ignore-whitespace.sh
@@ -111,7 +111,6 @@ sed -e 's/T//g' main.c.final \EOF
#include stdio.h
void print_int(int num);
-T/* a comment */
int func(int num);
int main() {
@@ -154,7 +153,8 @@ test_expect_success 'patch2 reverse applies with
--ignore-space-change' '
git config apply.ignorewhitespace change
test_expect_success 'patch2 applies (apply.ignorewhitespace = change)' '
- git apply patch2.patch
+ git apply patch2.patch
+ test_cmp main.c.final main.c
'
test_expect_success 'patch3 fails (missing string at EOL)' '
@@ -165,12 +165,8 @@ test_expect_success 'patch4 fails (missing EOL at EOF)' '
test_must_fail git apply patch4.patch
'
-test_expect_success 'patch5 applies (leading whitespace)' '
- git apply patch5.patch
-'
-
-test_expect_success 'patches do not mangle whitespace' '
- test_cmp main.c main.c.final
+test_expect_success 'patch5 fails (leading whitespace differences matter)' '
+ test_must_fail git apply patch5.patch
'
test_expect_success 're-create file (with --ignore-whitespace)' '
--
1.9.1-497-g6f5ca27
--
To unsubscribe from this list: send the line 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 14/19] tree-diff: rework diff_tree interface to be sha1 based
Kirill Smelkov k...@navytux.spb.ru writes: On Tue, Mar 25, 2014 at 10:46:32AM -0700, Junio C Hamano wrote: Kirill Smelkov k...@navytux.spb.ru writes: What are the downsides of __ prefix by the way? Aren't these names reserved for compiler/runtime implementations? Yes, but there are precedents when people don't obey it widely and in practice everything works :) I think you are alluding to the practice in the Linux kernel, but their requirement is vastly different---their product do not even link with libc and they always compile with specific selected versions of gcc, no? Let it be something portable anyway - how about diff_tree_sha1_low() ? Sure. As this is a file-scope static, I do not think the exact naming matters that much. Just FYI, we seem to use ll_ prefix (standing for low-level) in some places. 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/RFC 0/6] reuse deltas found by bitmaps
Jeff King p...@peff.net writes: Just looking at the 128-day case again, using bitmaps increased our server CPU time _and_ made a much bigger pack. This series not only fixes the CPU time regression, but it also drops the server CPU time to almost nothing. That's a nice improvement, and it makes perfect sense: we are reusing on-disk deltas instead of on-the-fly computing deltas against the preferred bases. True. I think we could still add the objects from the tip of the client's HAVE list. That should make the result at least per to the non-bitmap case, right? This patch would still be a CPU win on top of that, because it would reduce the number of objects which need a delta search in the first place. Yes. So I think the next steps are probably: 1. Measure the all objects are preferred bases approach and confirm that it is bad. ;-) 2. Measure the reused deltas become preferred bases approach. I expect the resulting size to be slightly better than what I have now, but not as good as v1.9.0's size (but taking less CPU time). Do you mean the bases for reused deltas become preferred bases, so that we can deltify more objects off of them? 3. Measure the figure out boundaries and add them as preferred bases, like we do without bitmaps approach. I expect this to behave similarly to v1.9.0. Yes. 4. Combine (2) and (3) and measure them. I'm _hoping_ this will give us the best of both worlds, but I still want to do the individual measurements so we can see where any improvement is coming from. Sensible. 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 1/6] t/perf-lib: factor boilerplate out of test_perf
Jeff King p...@peff.net writes:
About half of test_perf() is boilerplate, and half is
actually related to running the perf test. Let's split it
into two functions, so that we can reuse the boilerplate in
future commits.
Signed-off-by: Jeff King p...@peff.net
---
t/perf/perf-lib.sh | 61
+++---
1 file changed, 35 insertions(+), 26 deletions(-)
These early steps somewhat conflict with another topic that is
stalled (due to having no real users) on 'pu'. I do not think we
would terribly mind dropping tg/perf-lib-test-perf-cleanup and have
it rebased if the author or somebody else wants to have it in my
tree later, but just FYI.
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index a8c9574..20f306a 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -150,8 +150,8 @@ exit $ret' 3 24
return $eval_ret
}
-
-test_perf () {
+test_wrapper_ () {
+ test_wrapper_func_=$1; shift
test_start_
test $# = 3 { test_prereq=$1; shift; } || test_prereq=
test $# = 2 ||
@@ -162,35 +162,44 @@ test_perf () {
base=$(basename $0 .sh)
echo $test_count $perf_results_dir/$base.subtests
echo $1 $perf_results_dir/$base.$test_count.descr
- if test -z $verbose; then
- printf %s perf $test_count - $1:
- else
- echo perf $test_count - $1:
- fi
- for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
- say 3 running: $2
- if test_run_perf_ $2
- then
- if test -z $verbose; then
- printf %s $i
- else
- echo * timing run
$i/$GIT_PERF_REPEAT_COUNT:
- fi
+ base=$perf_results_dir/$perf_results_prefix$(basename $0
.sh).$test_count
+ $test_wrapper_func_ $@
+ fi
+
+ test_finish_
+}
+
+test_perf_ () {
+ if test -z $verbose; then
+ printf %s perf $test_count - $1:
+ else
+ echo perf $test_count - $1:
+ fi
+ for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
+ say 3 running: $2
+ if test_run_perf_ $2
+ then
+ if test -z $verbose; then
+ printf %s $i
else
- test -z $verbose echo
- test_failure_ $@
- break
+ echo * timing run $i/$GIT_PERF_REPEAT_COUNT:
fi
- done
- if test -z $verbose; then
- echo ok
else
- test_ok_ $1
+ test -z $verbose echo
+ test_failure_ $@
+ break
fi
- base=$perf_results_dir/$perf_results_prefix$(basename $0
.sh).$test_count
- $TEST_DIRECTORY/perf/min_time.perl test_time.* $base.times
+ done
+ if test -z $verbose; then
+ echo ok
+ else
+ test_ok_ $1
fi
- test_finish_
+ $TEST_DIRECTORY/perf/min_time.perl test_time.* $base.times
+}
+
+test_perf () {
+ test_wrapper_ test_perf_ $@
}
# We extend test_done to print timings at the end (./run disables this
--
To unsubscribe from this list: send the line 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 v8 01/12] Add data structures and basic functions for commit trailers
Christian Couder chrisc...@tuxfamily.org writes: Subject: Re: [PATCH v8 01/12] Add data structures and basic functions for commit trailers As pointed out many times for GSoC microprojects students, limit the scope with area: prefix for the commit title, e.g. Subject: trailers: add data structures and basic functions Please also refer to what has already been queued on 'pu' to avoid wasting review bandwidth and mark patches that are unchanged as such (but do send them to the list for review, so that people who haven't seen the previous round can also comment). As far as I can tell, this is the same as 8d1c70e5 (trailers: add data structures and basic functions, 2014-03-06), so I'll queue the remainder on top of that commit already on 'pu', which incidentally will preserve the original author timestamp from the previous incarnation. -- To unsubscribe from this list: send the line 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 v8 00/12] Add interpret-trailers builtin
Christian Couder chrisc...@tuxfamily.org writes: Until now git commit has only supported the well known Signed-off-by: trailer, that is used by many projects like the Linux kernel and Git. It is better to implement features for these trailers first in a new command rather than in builtin/commit.c, because this way the prepare-commit-msg and commit-msg hooks can reuse this command. The first is somewhat questionable. It is better to keep builtin/commit.c uncontaminated by any more hard-wired logic, like what we have for the signed-off-by line. Any new things can and should be doable in hooks, and this filter would help writing these hooks. And that is why the design goal of the filter is to make it at least as powerful as the built-in logic we have for signed-off-by lines; that would allow us to later eject the hard-wired logic for signed-off-by line from the main codepath, if/when we wanted to. Alternatively, we could build a library-ish API around this filter code and replace the hard-wired logic for signed-off-by line with a call into that API, if/when we wanted to, but that requires (in addition to the at least as powerful as the built-in logic) that the implementation of this stand-alone filter can be cleanly made into a reusable library, so that is a bit higher bar to cross than everything can be doable with hooks alternative. 3) Changes since version 7, thanks to Junio: * improved handling of empty trailer token * clearer way to create 'expected' files in tests * other small test cleanups * improved commit message * new way to parse config keys * strcasecmp() is not used anymore in some config related functions It is unclear which of the 12 patches are unchanged since the last round. Are reviewers expected to re-read all of them? Some values from the config file are lowercased instead. To enable that a new patch (3/12) is introduced to rationalize lowercase related functions. I am not very happy with these changes. I can see why you are not very happy. Perhaps it may make you happier if you did not move lowercase() at all, did the xstrdup_tolower() in a cleaner and more efficient way, and only used the latter in the 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
Re: [PATCH v8 03/12] Move lower case functions into wrapper.c
Christian Couder chrisc...@tuxfamily.org writes:
diff --git a/wrapper.c b/wrapper.c
index 0cc5636..c46026a 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -455,3 +455,17 @@ struct passwd *xgetpwuid_self(void)
errno ? strerror(errno) : _(no such user));
return pw;
}
+
+void lowercase(char *p)
+{
+ for (; *p; p++)
+ *p = tolower(*p);
+}
+
+char *xstrdup_tolower(const char *str)
+{
+ char *dup = xstrdup(str);
+ lowercase(dup);
+ return dup;
+}
+
As a pure code-movement step, this may be OK, but I am not sure if
both of them want to be public functions in this shape.
Perhaps
char *downcase_copy(const char *str)
{
char *copy = xmalloc(strlen(str) + 1);
int i;
for (i = 0; str[i]; i++)
copy[i] = tolower(str[i]);
copy[i] = '\0';
return copy;
}
may avoid having to copy things twice. Do you need the other
function exposed?
--
To unsubscribe from this list: send the line 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] test-lib: Document short options in t/README
Ilya Bobyr ilya.bo...@gmail.com writes:
If there is decision on how shortening should work for all the
options, maybe I could add a paragraph on that and make existing
options more consistent.
We should strive to make the following from gitcli.txt apply
throughout the system:
* many commands allow a long option `--option` to be abbreviated
only to their unique prefix (e.g. if there is no other option
whose name begins with `opt`, you may be able to spell `--opt` to
invoke the `--option` flag), but you should fully spell them out
when writing your scripts; later versions of Git may introduce a
new option whose name shares the same prefix, e.g. `--optimize`,
to make a short prefix that used to be unique no longer unique.
If so, '--valgrind' becomes impossible to shorten because there
is '--valgrind-only' that is a separate option. Same for
'--verbose' and '--verbose-only'.
Correct. If you really cared, --valgrind={yes,no,only} would be (or
have been) a better possibility, though.
--
To unsubscribe from this list: send the line 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/RFC 0/6] reuse deltas found by bitmaps
Jeff King p...@peff.net writes: But for a small fetch... 5311.3: server (1 days)0.20(0.17+0.03) 4.39(4.03+6.59) +2095.0% 5311.4: size (1 days) 57.2K 59.5K +4.1% 5311.5: client (1 days)0.08(0.08+0.00) 0.08(0.08+0.00) +0.0% Nice ;-) So this is a dead end, but I think it was good to double-check that. 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 v8 03/12] Move lower case functions into wrapper.c
Christian Couder chrisc...@tuxfamily.org writes: Yeah, but it seems a bit wasteful to allocate memory for a new string, then downcase it, then compare it with strcmp() and then free it, instead of just using strcasecmp() on the original string. I wasn't looking at the caller (and I haven't). I agree that, if you have to compare case-insensitive user input against known set of tokens, using strcasecmp() would be saner than making a downcased copy and the set of known tokens. I do not know however you want to compare in a case-insensitive way in your application, though. -- To unsubscribe from this list: send the line 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] MSVC: added missing include so `make INLINE=__inline` is no longer required
Marat Radchenko ma...@slonopotamus.org writes: Signed-off-by: Marat Radchenko ma...@slonopotamus.org --- xdiff/xutils.c | 1 + 1 file changed, 1 insertion(+) diff --git a/xdiff/xutils.c b/xdiff/xutils.c index 62cb23d..a21a835 100644 --- a/xdiff/xutils.c +++ b/xdiff/xutils.c @@ -23,6 +23,7 @@ #include limits.h #include assert.h #include xinclude.h +#include git-compat-util.h This is unfortunate for a few reasons: - xdiff/* is a borrowed code; we do not want to have (or add more) dependencies on the rest of Git, including compat-util. - When a piece of our code needs our compatibility support, compat-util must be the first header file included (either directly, or indirectly by including another header file that includes it at the top). My gut feeling is that adding a mechanism to add -DINLINE=__inline only on MSVC to the top-level Makefile, without touching this file, may be a much more palatable. I dunno. -- To unsubscribe from this list: send the line 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] patch-id: make it stable against hunk reordering
Michael S. Tsirkin m...@redhat.com writes: Patch id changes if you reorder hunks in a diff. If you reorder hunks, the patch should no longer apply [*1*], so a feature to make patch-id stable across such move would have no practical use ;-), but I am guessing you meant something else. Perhaps this is about using -O orderfile option, even though you happened to have implemented the id mixing at per-hunk level? [Footnote] *1* It has been a long time since I looked at the code, and I do not know offhand if git apply has such a bug not to diagnose a hunk for a file for an earlier part that comes later in its input stream after seeing another hunk for the same file as a bug. If it does not, perhaps we should. -- To unsubscribe from this list: send the line 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: Possible regression in master? (submodules without a master branch)
Johan Herland jo...@herland.net writes: I just found a failure to checkout a project with submodules where there is no explicit submodule branch configuration, and the submodules happen to not have a master branch: git clone git://gitorious.org/qt/qt5.git qt5 cd qt5 git submodule init qtbase git submodule update In current master, the last command fails with the following output: ... and with a bug-free system, what does it do instead? Just clone 'qtbase' and make a detached-head checkout at the commit recorded in the superproject's tree, or something else? Cloning into 'qtbase'... remote: Counting objects: 267400, done. remote: Compressing objects: 100% (61070/61070), done. remote: Total 267400 (delta 210431), reused 258876 (delta 202642) Receiving objects: 100% (267400/267400), 136.23 MiB | 6.73 MiB/s, done. Resolving deltas: 100% (210431/210431), done. Checking connectivity... done. error: pathspec 'origin/master' did not match any file(s) known to git. Unable to setup cloned submodule 'qtbase' Bisection points to 23d25e48f5ead73c9ce233986f90791abec9f1e8 (W. Trevor King: submodule: explicit local branch creation in module_clone). Looking at the patch, it seems to introduce an implicit assumption on the submodule origin having a master branch. Is this an intended change in behaviour? If an existing set-up that was working in a sensible way is broken by a change that assumes something that should not be assumed, then that is a serious regression, I would have to say. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git feature request: Option to force Git to abort a checkout if working directory is dirty (i.e. disregarding the check for conflicts)
Jonas Bang em...@jonasbang.dk writes: Hi Git developers, This is my first Git feature request, I hope it won’t get me hanged on the gallows ;o) *Git feature request:* Add an option to Git config to configure the criteria for when a git checkout should abort. *Name proposal and options:* checkout.clean false default checkout.clean true A configuration variable without command line override will make the system unusable. When thinking about a new feature, please make it a habit to first add a command line option and then if that option turns out to be useful in the real world (not in the imaginary world in which you had such a feature, where even you haven't lived in yet), then think about also adding configuration variables to control the default. Also, a useful definition of clean-ness may have to change over time as we gain experience with the feature. And also as a user personally gains experience with using Git. It is somewhat implausible that a boolean true/false may remain sufficient. *False behavior:* What is false? Ah, when the configuration is set to false, which will be the default? As is: When doing a checkout then Git will check if your working directory is dirty, and if so check if the checkout will result in any conflicts, and if so abort the checkout with a message: $ git checkout some_branch error: Your local changes to the following files would be overwritten by checkout: some_file Please, commit your changes or stash them before you can switch branches. Aborting If no conflicts then: $ git checkout some_branch M some_file M some_other_file Switched to branch 'some_branch' I.e. it will only abort if there are conflicts. Sensible. This is the behaviour that is very often depended upon by people who use Git with multiple branches. Are you thinking about changing it in any way when the new configuration is set to false, or is the above just a summary of what happens in the current system? *True behavior:* When doing a checkout then Git will check if your working directory is dirty (checking for both modified and added untracked files), and if so abort the checkout with a message: $ git checkout some_branch error: Your working directory is not clean. Please, commit your changes or stash them before you can switch branches. Aborting I.e. it will abort if working directory is dirty (checking for both modified and added untracked files). I.e. you can only do checkout if you get nothing to commit, working directory clean when running git status (ignoring ignored files though). The above two say very different things. For some people, having many throw away untracked files is a norm that they do not consider it is even worth their time to list them in .gitignore and they do not want to be reminded in git status output, and the latter definition of checkout.clean=true will kill checkout when status says there are some things that could be committed would suit them, while the former definition checkout.clean=true will kill checkout when there is any untracked files would be totally useless. So I can understand the latter, but I do not see how the former could be a useful addition. For some people it is also a norm to keep files that have been modified from HEAD and/or index without committing for a long time (e.g. earlier, Linus said that the version in Makefile is updated and kept modified in the working tree long before a new release is committed with that change). The default behaviour would cover their use case so your proposal would not hurt them, but I wonder if there are things you could do to help them as well, perhaps by allowing this new configuration to express something like local changes in these paths are except from this new check. I dunno. -- To unsubscribe from this list: send the line 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] patch-id: make it stable against hunk reordering
Michael S. Tsirkin m...@redhat.com writes: I started to remove that code, but then I recalled why I did it like this. There is a good reason. Yes, you can't simply reorder hunks just like this. But you can get the same effect by prefixing the header: Yes, that is one of the things I personally have on the chopping block. Having to deal with more than occurrences of the same pathname in the input made things in builtin/apply.c unnecessarily complex and I do not see a real gain for being able to concatenate two patches and feed it into a single git apply invocation. -- To unsubscribe from this list: send the line 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: Possible regression in master? (submodules without a master branch)
Jens Lehmann jens.lehm...@web.de writes: Am 27.03.2014 16:52, schrieb W. Trevor King: On Thu, Mar 27, 2014 at 03:21:49PM +0100, Johan Herland wrote: I just found a failure to checkout a project with submodules where there is no explicit submodule branch configuration, and the submodules happen to not have a master branch: The docs say [1]: A remote branch name for tracking updates in the upstream submodule. If the option is not specified, it defaults to 'master'. But the branch setting isn't configured for Qt, the .gitmodules file contains only this: [submodule qtbase] path = qtbase url = ../qtbase.git ... which is what we do now. Working around that to default to the upstream submodule's HEAD is possible (you can just use --branch HEAD), but I think it's easier to just explicitly specify your preferred branch. That is *not* easier, as Johan did not have to do that before. I think your patch 23d25e48f5ead73c9ce233986f90791abec9f1e8 does not do what the commit message promised: With this change, folks cloning submodules for the first time via: $ git submodule update ... will get a local branch instead of a detached HEAD, unless they are using the default checkout-mode updates. And Qt uses the default checkout-mode updates and doesn't have branch configured either. So we are facing a serious regression here. There are two potential issues (and a half) then: - When cloning with the default checkout-mode updates, the new feature to avoid detaching the HEAD should not kick in at all. - For a repository that does not have that branch thing configured, the doc says that it will default to 'master'. I do not think this was brought up during the review, but is it a sensible default if the project does not even have that branch? What are viable alternatives? - use 'master' and fail just the way Johan saw? - use any random branch that happens to be at the same commit as what is being checked out? - use the branch clone for the submodule repository saw the upstream was pointing at with its HEAD? - something else? - Johan's set-up was apparently not covered in the addition to t/ in 23d25e48 (submodule: explicit local branch creation in module_clone, 2014-01-26)---otherwise we would have caught this regression. Are there other conditions that are not covered? -- To unsubscribe from this list: send the line 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 14/19] tree-diff: rework diff_tree interface to be sha1 based
Kirill Smelkov k...@navytux.spb.ru writes: (please keep author email) 8 From: Kirill Smelkov k...@mns.spb.ru Date: Mon, 24 Feb 2014 20:21:46 +0400 Subject: [PATCH v3a] tree-diff: rework diff_tree interface to be sha1 based git am -c will discard everything above the scissors and then start parsing the in-body headers from there, so the above From: will be used. But you have a few entries in .mailmap; do you want to update them as well? By the way, in general I do not appreciate people lying on the Date: with an in-body header in their patches, either in the original or in rerolls. 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: Possible regression in master? (submodules without a master branch)
W. Trevor King wk...@tremily.us writes:
On Thu, Mar 27, 2014 at 06:31:27PM +0100, Jens Lehmann wrote:
Am 27.03.2014 18:16, schrieb Junio C Hamano:
Johan Herland jo...@herland.net writes:
I just found a failure to checkout a project with submodules where
there is no explicit submodule branch configuration, and the
submodules happen to not have a master branch:
git clone git://gitorious.org/qt/qt5.git qt5
cd qt5
git submodule init qtbase
git submodule update
In current master, the last command fails with the following output:
... and with a bug-free system, what does it do instead? Just clone
'qtbase' and make a detached-head checkout at the commit recorded in
the superproject's tree, or something else?
After reverting 23d25e48f5ead73 on current master it clones 'qtbase'
nicely with a detached HEAD.
Fixing this for initial update clone is pretty easy, we just need to
unset start_point before calling module_clone if
submodule.name.branch is not set.
There is this bit for update in git-submodule.txt:
For updates that clone missing submodules, checkout-mode updates
will create submodules with detached HEADs; all other modes will
create submodules with a local branch named after
submodule.path.branch.
[side note] Isn't that a typo of submodule.name.branch?
So the proposed change is to make the part before semicolon true?
If we are not newly cloning (because we already have it), if the
submodule.name.branch is not set *OR* refers to a branch that does
not even exist, shouldn't we either (1) abort as an error, or (2) do
the same and detach?
However, that's just going to
push remote branch ambiguity problems back to the --remote update
functionality. What should happen when submodule.name.branch is not
set and you run a --remote update, which has used:
git rev-parse ${remote_name}/${branch}
since the submodule.name.branch setting was introduced in 06b1abb
(submodule update: add --remote for submodule's upstream changes,
2012-12-19)?
Isn't --remote about following one specific branch the user who
issues that command has in mind? If you as the end user did not
give any indication which branch you meant, e.g. by leaving the
submodule.name.branch empty, shouldn't that be diagnosed as an
error?
gitmodules(5) is pretty clear that 'submodule.name.branch' defaults
to master (and not upstream's HEAD), do we want to adjust this at the
same time?
That may be likely. If the value set to a configuration variable
causes an established behaviour of a program change a lot, silently
defaulting that variable to something many people are expected to
have (e.g. 'master') would likely to cause a usability regression.
If an existing set-up that was working in a sensible way is broken
by a change that assumes something that should not be assumed,
then that is a serious regression, I would have to say.
Yes, especially as it promised to not change this use case.
Sorry. A side effect of relying too much on our existing
documentation and not enough on testing actual use cases. I can work
up some non-master submodule tests to go with the fix.
I was wondering if we need to revert the merge with that
branch out of 'master', or submodule folks can work on a set of
fixes to apply on top.
Will wait to see how it goes. 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: submodule.path.branch vs. submodule.name.branch
W. Trevor King wk...@tremily.us writes: [side note] Isn't that a typo of submodule.name.branch? Good catch. The transition from submodule.path.* to submodule.name.* happened in 73b0898d (Teach git submodule add the --name option, 2012-09-30), which landed in v1.8.1-rc0 on 2012-12-03. Thanks for digging. Strictly speaking, I think this was not even a transition (rather, there was no way to give a submodule a name that is different from its path). In any version of git whose git-submodule.sh has module_name helper function, the path and the name were conceptually two different things, and we should have been using the name, not path, throughout. ... Both should be updated to submodule.name.branch. I agree. 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 v8 03/12] Move lower case functions into wrapper.c
Junio C Hamano gits...@pobox.com writes: Christian Couder chrisc...@tuxfamily.org writes: Yeah, but it seems a bit wasteful to allocate memory for a new string, then downcase it, then compare it with strcmp() and then free it, instead of just using strcasecmp() on the original string. I wasn't looking at the caller (and I haven't). I agree that, if you have to compare case-insensitive user input against known set of tokens, using strcasecmp() would be saner than making a downcased copy and the set of known tokens. I do not know however you want to compare in a case-insensitive way in your application, though. It appears that one place this lowercase is used is to allow rAnDOm casing in the configuration file, e.g. [trailer Signed-off-by] where = AfTEr which I find is totally unnecessary. Do we churn code to accept such a nonsense input in other places? -- To unsubscribe from this list: send the line 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 v8 03/12] Move lower case functions into wrapper.c
Jeff King p...@peff.net writes: All bool config values allow tRuE. I was expecting somebody will bring it up, but think about it. Bool is a very special case. Even among CS folks, depending on your background, true may be True may be TRUE may be 1. Conflating it with some random enum does not make a good argument. Ones that take auto often use strcasecmp (e.g., diff.*.binary). blame.date and help.format choose from a fixed set of tokens, but use strcmp. I would say that the latter is the right thing to do. In general I do not see any reason _not_ to use strcasecmp for config values that are matching a fixed set. It's friendlier to the user,... Actually, I think it ends up being hostile to the users to accept random cases without a good reason. If you see two trailer elements whose where are specified as after and AFTER in somebody's configuration file, wouldn't that give a wrong impression that a new line with the latter somehow has a stronger desire to come later than the former? If you consistently take only the fixed strings, you do not have to worry about many people writing the same things in different ways, confusing each other. I would however fully agree with you that using strcasecmp() would be the cleanest when reading and maintaining the code **IF** we want to accept values in random case, but I do not agree that accepting random cases is a good thing, so... -- To unsubscribe from this list: send the line 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: Borrowing objects from nearby repositories
Andrew Keller and...@kellerfarm.com writes: Okay, so to re-frame my idea, like you said, the goal is to find a user- friendly way for the user to tell git-clone to set up the alternates file (or perhaps just use the --alternates parameter), and run a repack, and disconnect the alternate. And yet, we still want to be able to use --reference on its own, because there are existing use cases for that. Here are a few possible action items that came out of this discussion: 1. Introduce a new --borrow option to git clone. The updates to the SYNOPSIS section may go like this: -'git clone' [--reference repository] ...other options... +'git clone' [[--reference|--borrow] repository] ...other options... The new option can be used instead of --reference and they will be mutually incompatible. The first implementation of the --borrow option would do the following: (1) run the same git clone with the same command line but replacing --borrow with --reference; if this fails, exit with the same failure. (2) in the resulting repository, run git repack -a -d; if this fails, remove the entire directory the first step created, and exit with failure. (3) remove .git/objects/info/alternates from the resulting repository and exit with success. and it may be acceptable as the final implementation as well. 2. Make git repack safer for the users of clone --reference who want to keep sharing objects from the original. - Introduce the repack.local configuration variable that can be set to either true or false. Missing variable defaults to false. - A repack that is run without -l option on the command line will pretend as if it was given -l from the command line if repack.local is set to true. Add repack --no-local option to countermand this configuration variable from the command line. - Teach git clone --reference (but not git clone --borrow) to set repack.local = true in the configuration of the resulting repository. -- To unsubscribe from this list: send the line 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: SSL_CTX leak?
Jeff King p...@peff.net writes: On Thu, Mar 27, 2014 at 10:37:07AM -0300, Thiago Farina wrote: Do we leak the context we allocate in imap-send.c:280 intentionally? It was never mentioned on the mailing list when the patches came originally, so I suspect is just an omission. Presumably the SSL_CTX is needed by the connection that survives after the function, but my reading of SSL_CTX_free implies that the data is reference-counted, and the library would presumably handle it fine. Yes, I was reading the SSL_new() yesterday and found out that at least in a recent code it increments the reference count on the ctx it is fed. So it would be the right thing to decrement the refcount in the caller that created the context and used to call SSL_new(), but I fully agree with the analysis below (with s/a huge/any/): OTOH, it is probably not causing a huge problem (since we wouldn't end up freeing it until the end of the program anyway), so I would not personally devote to many brain cycles to figuring out how OpenSSL handles it. Heh. So you are saying that I wasted 30 minutes yesterday? ;-) 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] t4212: handle systems with post-apocalyptic gmtime
Jeff King p...@peff.net writes: Sat Jan 25 10:46:39 316889355 -0700 9 Wed Sep 6 02:46:39 -1126091476 -0700 99 Thu Oct 24 18:46:39 1623969404 -0700 Thanks. Given the value where it fails, it kind of looks like there is some signed 32-bit value at work (~300 million years is OK, but 10 times that, rather than yielding ~3 billion, gets us -1 billion). Perhaps tm.tm_year is 32-bit. That is what it looks like to me, too. It makes me wonder if some other platforms may have similar breakage using 16-bit signed tm_year and how such a breakage would look like, though ;-) So what do we want to do? I think the options are: 1. Try to guess when we have a bogus timestamp value with an arbitrary cutoff like greater than 1 million years from now (and enforce it via time_t seconds, and avoid gmtime entirely). That is made-up and arbitrary, but it also is sufficiently far that it won't ever matter, and sufficiently close that any gmtime should behave sensibly with it. I think that two assumptions here are that (1) we would be able to find a single insane value (like 3 billion years from now expressed in time_t seconds) the test script would be able to feed and expect it to fail on all platforms we care about, even though how exactly that value causes various platforms fail may be different (glibc may give us a NULL from gmtime, FreeBSD may leave their own sentinel in gmtime we can recognize, and some others may simply wrap around the years); and that (2) the only other class of failure mode we haven't considered bevore Charles's report is tm_year wrapping around 32-bit signed int. Offhand, the three possible failure modes this thread identified sounds to me like the only plausible ones, and I think the best way forward might be to - teach the is the result sane, even though we may have got a non-NULL from gmtime? otherwise let's signal a failure by replacing it with a known sentinel value codepath the new failure mode Charles's report suggests---if we feed a positive timestamp and gmtime gave us back a tm_year+1900 0, that is certainly an overflow; and - Use that 3-billion years timestamp from Charles's report in the test and make sure we convert it to the known sentinel value. perhaps? 2. Accept that we can't guess at every broken gmtime's output, and just loosen the test to make sure we don't segfault. Of course that is a simpler option, but we may have identified all plausible failure modes, in which case we can afford to go with your original plan to validate that we not just avoid segfaulting on one of the three failure modes from gmtime, but also cover the other two failure modes and signal any of them with a sentinel. That way may allow us to identify the fourth failure mode we haven't anticipated. -- To unsubscribe from this list: send the line 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: [RFC] submodule: change submodule.name.branch default from master to HEAD
Jens Lehmann jens.lehm...@web.de writes: Am 28.03.2014 04:58, schrieb W. Trevor King: On Thu, Mar 27, 2014 at 08:52:55PM -0700, W. Trevor King wrote: On Thu, Mar 27, 2014 at 11:43:47PM -0400, Eric Sunshine wrote: On Thu, Mar 27, 2014 at 11:36 PM, W. Trevor King wk...@tremily.us wrote: submodule.name.branch:: A remote branch name for tracking updates in the upstream submodule. - If the option is not specified, it defaults to 'master'. See the - `--remote` documentation in linkgit:git-submodule[1] for details. + If the option is not specified, it defaults to the subproject's Did you mean s/subproject/submodule/ ? + HEAD. See the `--remote` documentation in linkgit:git-submodule[1] + for details. No the remote branch is in the upstream subproject. I suppose I meant “the submodule's remote-tracking branch following the upstream subproject's HEAD which we just fetched so it's fairly current” ;). Hmm, maybe we should change the existing “upstream submodule” to “upstream subproject” for consistency? For me it's still an upstream submodule ... I inherited the habit of saying submodule vs superproject from Linus (I think) during the very early days, and there is no such thing as subproject or supermodule in my vocabulary. Just one documentation-reader's opinion, not an edict from the maintainer. -- To unsubscribe from this list: send the line 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 v8 03/12] Move lower case functions into wrapper.c
Jeff King p...@peff.net writes: But I also do not overly care. Literally zero people have complained that [log]date = RFC822 is not accepted, so it is probably not a big deal either way. That is most likely because we do not advertise these enum values spelled in random cases in our documentation and people do not even attempt to upcase the examples they see. So you are right to say that it does not hurt anybody in practice if the code does not strcasecmp() input from them. We do not know if using strcasecmp() there and allowing them to feed the enums spelled in random cases would invite confusions in the longer run, so let's not risk it. There is no upside in using strcasecmp() here. By the way, that is rfc2822---do we want rfc822 as its synonym as well as rfc, I wonder ;-) -- To unsubscribe from this list: send the line 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] gitweb: gpg signature status indication for commits
Victor Kartashov v.kartas...@npo-echelon.ru writes:
show gpg signature (if any) for commit message in gitweb
in case of valid signature highlight it with green
in case of invalid signature highlight it with red
If that is a single sentence, please write it as such:
Show gpg signature (if any) for commit message in gitweb in case of
valid signature highlight it with green in case of invalid signature
highlight it with red.
But that is almost unparsable ;-)
Teach gitweb to show GPG signature verification status when
showing a commit that is signed. Highlight in green or red to
differentiate valid and invalid signatures.
or something?
Is it a good idea to do this unconditionally in all repositories?
Signed-off-by: Victor Kartashov victor.kartas...@gmail.com
---
gitweb/gitweb.perl | 36 +---
gitweb/static/gitweb.css | 11 +++
2 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 79057b7..ccde90f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3430,8 +3430,9 @@ sub parse_commit_text {
my ($commit_text, $withparents) = @_;
my @commit_lines = split '\n', $commit_text;
my %co;
+ my @signature = ();
- pop @commit_lines; # Remove '\0'
+ pop @commit_lines if ($commit_lines[-1] =~ \0); # Remove '\0'
This comment, which only says what it intends to do without saying
why it wants to do so, does not explain why this is a good change.
Does the code even know if $commit_lines[-1] is a non-empty string?
Is it safe to assume if $commit_lines[-1] has a NUL anywhere, it is
always the last line that ends the record, which is not part of the
commit log message?
I am assuming that this $commit_text is read from log -z (or
rev-list -z) output and split at NUL boundary, but if that is the
case, it might be cleaner to remove the trailing NUL from $commit_text
before even attempting to split it into an array at LFs, but that is
an unrelated tangent.
A bigger question is why does the incoming data sometimes ends with
NUL that must be stripped out, and sometimes does not? I see that
you are updating the data producer in the later part of the patch;
wouldn't it be saner if you have the data producer produce the input
to this function in a way that is consistent with the current code,
i.e. always terminate the output with a NUL?
@@ -3469,6 +3470,9 @@ sub parse_commit_text {
$co{'committer_name'} = $co{'committer'};
}
}
+ elsif ($line =~ /^gpg: /) {
I think Eric already pointed out the style issue on this line.
@@ -3508,6 +3512,10 @@ sub parse_commit_text {
foreach my $line (@commit_lines) {
$line =~ s/^//;
}
+ push(@commit_lines, ) if scalar @signature;
+ foreach my $sig (@signature) {
+ push(@commit_lines, $sig);
+ }
Hmm, is it different from doing:
push @commit_lines, @signature;
in some way?
@@ -4571,7 +4579,21 @@ sub git_print_log {
# print log
my $skip_blank_line = 0;
foreach my $line (@$log) {
- if ($line =~ m/^\s*([A-Z][-A-Za-z]*-[Bb]y|C[Cc]): /) {
+ if ($line =~ m/^gpg:(.)+Good(.)+/) {
+ if (! $opts{'-remove_signoff'}) {
Sorry, but I fail to see what the remove-signoff option has to do
with this new feature. The interaction needs to be explained in the
log message.
+ print span class=\good_sign\ .
esc_html($line) . /spanbr/\n;
+ $skip_blank_line = 1;
+ }
+ next;
+ }
+ elsif ($line =~ m/^gpg:(.)+BAD(.)+/) {
+ if (! $opts{'-remove_signoff'}) {
+ print span class=\bad_sign\ .
esc_html($line) . /spanbr/\n;
+ $skip_blank_line = 1;
+ }
+ next;
+ }
+ elsif ($line =~ m/^\s*([A-Z][-A-Za-z]*-[Bb]y|C[Cc]): /) {
if (! $opts{'-remove_signoff'}) {
print span class=\signoff\ .
esc_html($line) . /spanbr/\n;
$skip_blank_line = 1;
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 3212601..e99e223 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -136,6 +136,17 @@ span.signoff {
color: #88;
}
+span.good_sign {
+ font-weight: bold;
+ background-color: #aaffaa;
+}
+
+span.bad_sign {
+ font-weight: bold;
+ background-color: #88;
+ color: #ff
+}
+
div.log_link {
padding: 0px 8px;
font-size: 70%;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: git commit vs. ignore-submodules
Jens Lehmann jens.lehm...@web.de writes: .. but it's less clear if one explicitely stages an updated submodule using git add. Git commit will ignore it anyway, if ignore=all is configured in .gitmodules. Maybe that's correct too That definitely smells like a bug to me. Excluding modified submodules when git add -u is run with ignore=all would be fine and most likely the right thing to do, but once the user actually adds the change to the index, it should not be ignored. ... And also, I'd like to know git folks' opinion on whether it's OK that git commit with ignore=all in .gitmodules ignores submodules even when they are explicitely staged with git add. No, they should be visible in status and commit when the user chose to add them. I see if I can hack something up (as I've been bitten myself by that recently ;-). 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] MSVC: fix t0040-parse-options
Marat Radchenko ma...@slonopotamus.org writes:
Signed-off-by: Marat Radchenko ma...@slonopotamus.org
---
test-parse-options.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/test-parse-options.c b/test-parse-options.c
index 434e8b8..7840493 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -11,6 +11,7 @@ static char *string = NULL;
static char *file = NULL;
static int ambiguous;
static struct string_list list;
+static const char *default_string = default;
That wastes 4 or 8 bytes compared to
static const char default_string[] = default;
no?
static int length_callback(const struct option *opt, const char *arg, int
unset)
{
@@ -60,7 +61,7 @@ int main(int argc, char **argv)
OPT_STRING('o', NULL, string, str, get another string),
OPT_NOOP_NOARG(0, obsolete),
OPT_SET_PTR(0, default-string, string,
- set string to default, (unsigned long)default),
+ set string to default, default_string),
OPT_STRING_LIST(0, list, list, str, add str to list),
OPT_GROUP(Magic arguments),
OPT_ARGUMENT(quux, means --quux),
I can see how this patch would not hurt, but at the same time, I
cannot see why this patch is a FIX. A string literal default is
a pointer to constant string, and being able to cast a pointer to
unsigned long is something that is done fairly commonly without
problems [*1*]. It needs to be explained why this change is needed
along the lines of...
We prepare an element in an array of struct option with
OPT_SET_PTR to point a variable to a literal string
default, but MSVC compiler fails to distim the doshes for
such and such reasons.
Work it around by moving the literal string outside the
definition of the struct option, which MSVC can understand
it.
in the log message.
[Footnote]
*1* The cast should actually be intptr_t for it to be kosher. I
also suspect that the cast should happen inside OPT_SET_PTR()
macro defintion, like in the attached patch.
parse-options.h | 2 +-
test-parse-options.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/parse-options.h b/parse-options.h
index d670cb9..7a24d2e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -129,7 +129,7 @@ struct option {
#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
NULL, 1}
#define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG, NULL, (p) }
+ (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p)
}
#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG|PARSE_OPT_NONEG,
NULL, (i) }
#define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(n),
(h) }
diff --git a/test-parse-options.c b/test-parse-options.c
index 434e8b8..10da63e 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -60,7 +60,7 @@ int main(int argc, char **argv)
OPT_STRING('o', NULL, string, str, get another string),
OPT_NOOP_NOARG(0, obsolete),
OPT_SET_PTR(0, default-string, string,
- set string to default, (unsigned long)default),
+ set string to default, default),
OPT_STRING_LIST(0, list, list, str, add str to list),
OPT_GROUP(Magic arguments),
OPT_ARGUMENT(quux, means --quux),
--
To unsubscribe from this list: send the line 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] patch-id: make it stable against hunk reordering
Michael S. Tsirkin m...@redhat.com writes: Patch id changes if you reorder hunks in a diff. Reording files is fine, and as we discussed, having multiple patches that touch the same path is fine, but do not sound as if you are allowing to reorder hunks inside a single patch that touch a single file. 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] MSVC: link in invalidcontinue.obj for better POSIX compatibility
Marat Radchenko ma...@slonopotamus.org writes: This patch fixes crashes caused by quitting from PAGER. Can you elaborate a bit more on the underlying cause, summarizing what you learned from this discussion, so that those who read git log output two weeks from now do not have to come back to this thread in the mail archive in order to figure out why we suddenly needs to link with yet another library? Thanks. Signed-off-by: Marat Radchenko ma...@slonopotamus.org --- Please do not cull the Cc list. That was gmane web interface. The correct solution is to link against invalidcontinue.obj in the MSVC build. This is a compiler-provided object file that changes the default behavior to the expected kind, i.e., C runtime functions return EINVAL when appropriate instead of crashing the application. Thanks for a hint. config.mak.uname | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.mak.uname b/config.mak.uname index 38c60af..8e7ec6e 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows) compat/win32/dirent.o COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\.exe\ BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib - EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib + EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj PTHREAD_LIBS = lib = ifndef DEBUG -- To unsubscribe from this list: send the line 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] t4212: handle systems with post-apocalyptic gmtime
Jeff King p...@peff.net writes:
On Fri, Mar 28, 2014 at 09:41:53AM -0700, Junio C Hamano wrote:
Offhand, the three possible failure modes this thread identified
sounds to me like the only plausible ones, and I think the best way
forward might be to
- teach the is the result sane, even though we may have got a
non-NULL from gmtime? otherwise let's signal a failure by
replacing it with a known sentinel value codepath the new
failure mode Charles's report suggests---if we feed a positive
timestamp and gmtime gave us back a tm_year+1900 0, that is
certainly an overflow; and
I don't think we can analyze the output from gmtime. If it wraps the
year at N, then won't N+2014 look like a valid value?
Yes, but I was hoping that there are small number of possible N's
;-)
If we are going to do something trustworthy I think it has to be before
we hand off to gmtime. Like:
diff --git a/date.c b/date.c
index e1a2cee..e0c43c4 100644
--- a/date.c
+++ b/date.c
@@ -57,6 +57,8 @@ static time_t gm_time_t(unsigned long time, int tz)
static struct tm *time_to_tm(unsigned long time, int tz)
{
time_t t = gm_time_t(time, tz);
+ if (t )
+ return NULL;
return gmtime(t);
}
I suspect that would handle the FreeBSD case, as well.
By the way, I have a suspicion that the gm_time_t above can overflow if
you specially craft a value at the edge of what time_t can handle (we
check that our value will not overflow time_t earlier, but now we might
be adding up to 86400 seconds to it). sigh
Yuck. Let's not go there.
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] MSVC: link in invalidcontinue.obj for better POSIX compatibility
Junio C Hamano gits...@pobox.com writes: Marat Radchenko ma...@slonopotamus.org writes: This patch fixes crashes caused by quitting from PAGER. Can you elaborate a bit more on the underlying cause, summarizing what you learned from this discussion, so that those who read git log output two weeks from now do not have to come back to this thread in the mail archive in order to figure out why we suddenly needs to link with yet another library? Thanks. Just to avoid getting misunderstood, I am not asking it to be explained to me in an e-mail. I want to see a patch with its proposed commit log message to explain it to readers of git log. 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 1/3] patch-id: make it stable against hunk reordering
Michael S. Tsirkin m...@redhat.com writes:
Patch id changes if you reorder hunks in a diff.
As the result is functionally equivalent, this is surprising to many
people.
In particular, reordering hunks is helpful to make patches
more readable (e.g. API header diff before implementation diff).
In git, it is often done e.g. using the -O orderfile option,
so supporting it better has value.
Hunks within file can be reordered manually provided
the same pathname can appear more than once in the input.
Change patch-id behaviour making it stable against
hunk reodering:
- prepend header to each hunk (if not there)
Note: POSIX requires patch to be robust against hunk reordering
provided each diff hunk has a header:
http://pubs.opengroup.org/onlinepubs/7908799/xcu/patch.html
If the patch file contains more than one patch, patch will
attempt to
apply each of them as if they came from separate patch files.
(In this
case the name of the patch file must be determinable for each
diff
listing.)
- calculate SHA1 hash for each hunk separately
- sum all hashes to get patch id
Add a new flag --unstable to get the historical behaviour.
Add --stable which is a nop, for symmetry.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
Changes from v1: documented motivation for supporting
hunk reordering (and not just file reordering).
No code changes.
Junio, you didn't respond so I'm not sure whether I convinced
you that supporting hunk reordering within file has value.
So I kept this functionality around for now, if
you think I should drop this, please let me know explicitly.
Thanks, and sorry about being dense!
The motivation I read from the exchange was that:
(1) we definitely want to support a mode that is stable with use of
diff -O (i.e. reordering patches per paths);
(2) supporting a patch with swapped hunks does not have any
practical value. When you have a patch to the same file F with
two hunks starting at lines 20 and 40, manually reordering
hunks to create a patch that shows the hunk that starts at line
40 and then the other hunk that starts at line 20 is not a
useful exercise;
(3) but supporting a patch that touches the same path more than
once is in line with what patch and git apply after
7a07841c (git-apply: handle a patch that touches the same path
more than once better, 2008-06-27) do.
In other words, the goal of this change would be to give the same id
for all these three:
(A) straight-forward:
diff --git a/foo.c b/foo.c
--- a/foo.c
+++ b/foo.c
@@ -20,1 +20,1 @@
-foo
+bar
@@ -40,1 +40,1 @@
-frotz
+xyzzy
(B) as two patches concatenated together:
diff --git a/foo.c b/foo.c
--- a/foo.c
+++ b/foo.c
@@ -20,1 +20,1 @@
-foo
+bar
diff --git a/foo.c b/foo.c
--- a/foo.c
+++ b/foo.c
@@ -40,1 +40,1 @@
-frotz
+xyzzy
(C) the same as (B) but with a swapped order:
diff --git a/foo.c b/foo.c
--- a/foo.c
+++ b/foo.c
@@ -40,1 +40,1 @@
-frotz
+xyzzy
diff --git a/foo.c b/foo.c
--- a/foo.c
+++ b/foo.c
@@ -20,1 +20,1 @@
-foo
+bar
Am I reading you correctly?
If that is the case, I think I can buy such a change. It appears to
me that in addition to changing the way the bytes form multiple
hunks are hashed, it would need to hash the file-level headers
together with each hunk when processing input (A) in order to make
the output consistent with the output for the latter two.
Alternatively, you could hash the header for the same path only once
when processing input like (B) or (C) and mix. That would also give
you the same result as processing (A) in a straight-forward way.
builtin/patch-id.c | 71
++
1 file changed, 55 insertions(+), 16 deletions(-)
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3cfe02d..253ad87 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,17 +1,14 @@
#include builtin.h
-static void flush_current_id(int patchlen, unsigned char *id, git_SHA_CTX *c)
+static void flush_current_id(int patchlen, unsigned char *id, unsigned char
*result)
{
- unsigned char result[20];
char name[50];
if (!patchlen)
return;
- git_SHA1_Final(result, c);
memcpy(name, sha1_to_hex(id), 41);
printf(%s %s\n, sha1_to_hex(result), name);
- git_SHA1_Init(c);
}
static int remove_space(char *line)
@@ -56,10 +53,30 @@ static int scan_hunk_header(const char *p, int *p_before,
int *p_after)
return 1;
}
-static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX
Re: [PATCH] t4212: handle systems with post-apocalyptic gmtime
Jeff King p...@peff.net writes: This (non-)issue has consumed a lot more brain power than it is probably worth. I'd like to figure out which patch to go with and be done. :) Let's just deal with a simple known cases (like FreeBSD) in the real code that everybody exercises at runtime, and have the new test only check we do not segfault on a value we used to segfault upon seeing. -- To unsubscribe from this list: send the line 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 14/19] tree-diff: rework diff_tree interface to be sha1 based
Johannes Sixt j...@kdbg.org writes: My reading of git-send-email is: * $time = time - scalar $#files prepares the initial timestamp, so that running two git send-email back to back will give timestamps to the series sent out by the first invocation that are older than the ones the second series will get; A completely irrelevant tangent, but I was being an idiot here. The -scaler #$files is not about two send-email running back to back. A second invocation that sends out a long series will start its timestamp #$files in the past, that will overlap with the timestamp of the last one in the first invocation. And that is not what the code attempts to address. It wants to merely avoid timestamps from the future. -- To unsubscribe from this list: send the line 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] MSVC: link in invalidcontinue.obj for better POSIX compatibility
Marat Radchenko ma...@slonopotamus.org writes: By default, Windows abort()'s instead of setting errno=EINVAL when invalid arguments are passed to standard functions. For example, when PAGER quits and git detects it with errno=EPIPE on write(), check_pipe() in write_or_die.c tries raise(SIGPIPE) but since there is no SIGPIPE on Windows, it is treated as invalid argument, causing abort() and crash report window. Linking in invalidcontinue.obj (provided along with MS compiler) allows raise(SIGPIPE) to return with errno=EINVAL. Signed-off-by: Marat Radchenko ma...@slonopotamus.org --- Thanks; will queue. config.mak.uname | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.mak.uname b/config.mak.uname index 38c60af..8e7ec6e 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -366,7 +366,7 @@ ifeq ($(uname_S),Windows) compat/win32/dirent.o COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\.exe\ BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib - EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib + EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib invalidcontinue.obj PTHREAD_LIBS = lib = ifndef DEBUG -- To unsubscribe from this list: send the line 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] patch-id: make it stable against hunk reordering
Michael S. Tsirkin m...@redhat.com writes:
+static void flush_one_hunk(unsigned char *result, git_SHA_CTX *ctx)
{
- int patchlen = 0, found_next = 0;
+ unsigned char hash[20];
+ unsigned short carry = 0;
+ int i;
+
+ git_SHA1_Final(hash, ctx);
+ git_SHA1_Init(ctx);
+ /* 20-byte sum, with carry */
+ for (i = 0; i 20; ++i) {
+ carry += result[i] + hash[i];
+ result[i] = carry;
+ carry = 8;
+ }
Was there a reason why bitwise xor is not sufficient for mixing
these two indenendent hashes? If the 20-byte sums do not offer
benefit over that, the code for bitwise xor would be certainly be
simpler, I would imagine?
+}
+static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,
+struct strbuf *line_buf, int stable)
+{
+ int patchlen = 0, found_next = 0, hunks = 0;
int before = -1, after = -1;
+ git_SHA_CTX ctx, header_ctx;
+
+ git_SHA1_Init(ctx);
+ hashclr(result);
while (strbuf_getwholeline(line_buf, stdin, '\n') != EOF) {
char *line = line_buf-buf;
@@ -99,6 +116,18 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
if (!memcmp(line, @@ -, 4)) {
/* Parse next hunk, but ignore line numbers. */
scan_hunk_header(line, before, after);
+ if (stable) {
+ if (hunks) {
+ flush_one_hunk(result, ctx);
+ memcpy(ctx, header_ctx,
+sizeof ctx);
+ } else {
+ /* Save ctx for next hunk. */
+ memcpy(header_ctx, ctx,
+sizeof ctx);
+ }
+ }
+ hunks++;
continue;
}
@@ -107,7 +136,10 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
break;
/* Else we're parsing another header. */
+ if (stable hunks)
+ flush_one_hunk(result, ctx);
before = after = -1;
+ hunks = 0;
}
/* If we get here, we're inside a hunk. */
@@ -119,39 +151,46 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
/* Compute the sha without whitespace */
len = remove_space(line);
patchlen += len;
- git_SHA1_Update(ctx, line, len);
+ git_SHA1_Update(ctx, line, len);
}
if (!found_next)
hashclr(next_sha1);
+ flush_one_hunk(result, ctx);
What I read from these changes is that you do not do anything
special about the per-file header, so two no overlapping patches
with a single hunk each that touches the same path concatenated
together would not result in the same patch-id as a single-patch
that has the same two hunks. Which would break your earlier 'Yes,
reordering only the hunks will not make sense, but before each hunk
you could insert the same diff --git a/... b/... to make them a
concatenation of patches that touch the same file', I would think.
Is that what we want to happen? Or is my reading mistaken?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Mar 2014, #07; Fri, 28)
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.
More topics merged to 'master', many of which are fallouts from GSoC
microprojects.
You can find the changes described here in the integration branches
of the repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--
[Graduated to master]
* ah/doc-gitk-config (2014-03-20) 1 commit
(merged to 'next' on 2014-03-20 at d671b60)
+ Documentation/gitk: document the location of the configulation file
* bg/rebase-off-of-previous-branch (2014-03-19) 1 commit
(merged to 'next' on 2014-03-21 at 916b759)
+ rebase: allow - short-hand for the previous branch
git rebase learned to interpret a lone - as @{-1}, the
branch that we were previously on.
* bp/commit-p-editor (2014-03-18) 7 commits
(merged to 'next' on 2014-03-21 at 23b6b06)
+ run-command: mark run_hook_with_custom_index as deprecated
+ merge hook tests: fix and update tests
+ merge: fix GIT_EDITOR override for commit hook
+ commit: fix patch hunk editing with commit -p -m
+ test patch hunk editing with commit -p -m
+ merge hook tests: use 'test_must_fail' instead of '!'
+ merge hook tests: fix missing '' in test
When it is not necessary to edit a commit log message (e.g. git
commit -m is given a message without specifying -e), we used to
disable the spawning of the editor by overriding GIT_EDITOR, but
this means all the uses of the editor, other than to edit the
commit log message, are also affected.
* fr/add-interactive-argv-array (2014-03-18) 1 commit
(merged to 'next' on 2014-03-20 at 9d65f3d)
+ add: use struct argv_array in run_add_interactive()
* jk/pack-bitmap (2014-03-17) 1 commit
(merged to 'next' on 2014-03-20 at bba6246)
+ pack-objects: turn off bitmaps when skipping objects
Instead of dying when asked to (re)pack with the reachability
bitmap when a bitmap cannot be built, just (re)pack without
producing a bitmap in such a case, with a warning.
* jk/pack-bitmap-progress (2014-03-17) 2 commits
(merged to 'next' on 2014-03-20 at c7a83f9)
+ pack-objects: show reused packfile objects in Counting objects
+ pack-objects: show progress for reused packfiles
The progress output while repacking and transferring objects showed
an apparent large silence while writing the objects out of existing
packfiles, when the reachability bitmap was in use.
* jk/subtree-prefix (2014-03-17) 1 commit
(merged to 'next' on 2014-03-20 at 81367fa)
+ subtree: initialize prefix variable
A stray environment variable $prefix could have leaked into and
affected the behaviour of the subtree script.
* ys/fsck-commit-parsing (2014-03-19) 2 commits
(merged to 'next' on 2014-03-21 at 2728983)
+ fsck.c:fsck_commit(): use skip_prefix() to verify and skip constant
+ fsck.c:fsck_ident(): ident points at a const string
--
[New Topics]
* jc/apply-ignore-whitespace (2014-03-26) 1 commit
- apply --ignore-space-change: lines with and without leading whitespaces do
not match
An RFC. --ignore-space-change option of git apply ignored the
spaces at the beginning of line too aggressively, which is
inconsistent with the option of the same name diff and git diff
have.
Will hold.
* jc/rev-parse-argh-dashed-multi-words (2014-03-24) 3 commits
- parse-options: make sure argh string does not have SP or _
- update-index: teach --cacheinfo a new syntax mode,sha1,path
- parse-options: multi-word argh should use dash to separate words
(this branch uses ib/rev-parse-parseopt-argh.)
Make sure that the help text given to describe the param part
of the git cmd --option=param does not contain SP or _,
e.g. --gpg-sign=key-id option for git commit is not spelled
as --gpg-sign=key id.
Will merge to 'next'.
* jk/commit-dates-parsing-fix (2014-03-26) 1 commit
- t4212: loosen far-in-future test for AIX
I think we agreed that a simpler test would be a better way
forward.
* mr/msvc-link-with-invalidcontinue (2014-03-28) 1 commit
- MSVC: link in invalidcontinue.obj for better POSIX compatibility
Will merge to 'next'.
* mr/msvc-link-with-lcurl (2014-03-27) 1 commit
(merged to 'next' on 2014-03-28 at 3281dab)
+ MSVC: allow linking with the cURL library
Will merge to 'master'.
* wt/doc-submodule-name-path-confusion-1 (2014-03-27) 1 commit
(merged to 'next' on 2014-03-28 at 225f241)
+ doc: submodule.* config are keyed by submodule names
Will merge to 'master'.
* wt/doc-submodule-name-path-confusion-2 (2014-03-27) 1 commit
(merged to 'next' on 2014-03-28 at ec5bcf3)
+ doc: submodule.*.branch config is keyed by name
Will merge to 'master'.
* ep/shell-command-substitution (2014-03-25) 2 commits
(merged to 'next' on 2014-03-28 at 99a512a)
+ git-am.sh: use the $(...) construct for command substitution
+ check-builtins.sh: use
Re: [PATCH v2 1/3] patch-id: make it stable against hunk reordering
Junio C Hamano gits...@pobox.com writes:
@@ -99,6 +116,18 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
if (!memcmp(line, @@ -, 4)) {
/* Parse next hunk, but ignore line numbers. */
scan_hunk_header(line, before, after);
+if (stable) {
+if (hunks) {
+flush_one_hunk(result, ctx);
+memcpy(ctx, header_ctx,
+ sizeof ctx);
+} else {
+/* Save ctx for next hunk. */
+memcpy(header_ctx, ctx,
+ sizeof ctx);
+}
+}
+hunks++;
continue;
}
@@ -107,7 +136,10 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
break;
/* Else we're parsing another header. */
+if (stable hunks)
+flush_one_hunk(result, ctx);
before = after = -1;
+hunks = 0;
}
/* If we get here, we're inside a hunk. */
@@ -119,39 +151,46 @@ static int get_one_patchid(unsigned char *next_sha1,
git_SHA_CTX *ctx, struct st
/* Compute the sha without whitespace */
len = remove_space(line);
patchlen += len;
-git_SHA1_Update(ctx, line, len);
+git_SHA1_Update(ctx, line, len);
}
if (!found_next)
hashclr(next_sha1);
+flush_one_hunk(result, ctx);
What I read from these changes is that you do not do anything
special about the per-file header, so two no overlapping patches
with a single hunk each that touches the same path concatenated
together would not result in the same patch-id as a single-patch
that has the same two hunks. Which would break your earlier 'Yes,
reordering only the hunks will not make sense, but before each hunk
you could insert the same diff --git a/... b/... to make them a
concatenation of patches that touch the same file', I would think.
Is that what we want to happen? Or is my reading mistaken?
Heh, I was blind---copying into header_ctx and then restoring that
in preparation for the next round is exactly for duplicating the
per-file header sum to each and every hunk, so you are already doing
that.
--
To unsubscribe from this list: send the line 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] MSVC: fix t0040-parse-options crash
Marat Radchenko ma...@slonopotamus.org writes:
On 64-bit MSVC, pointers are 64 bit but `long` is only 32.
Thus, casting string to `unsigned long`, which is redundand on other
platforms, throws away important bits and when later cast to `intptr_t`
results in corrupt pointer.
This patch fixes test-parse-options by replacing harming cast with
correct one.
Signed-off-by: Marat Radchenko ma...@slonopotamus.org
---
Aargh! Didn't notice that V2 introduced compilation warning. Take three.
I am glad that I asked you to clarify, as I totally forgot that
there are L32P64 boxes.
I love it every time to see an attempt to describe why the solution
works clearly results in a better patch. It is not about writing
verbose log message; it is about thinking things through and clearly
cut to the core of the issue. Moving the string literal to a
separate variable to be used in the constructor in v1 was totally a
red-herring. Your updated log message makes it crystal clear that
using the correct typecast, not unsigned long but intptr_t, is
the core of the solution.
As OPT_SET_PTR() is about setting the pointer value to intptr_t defval,
a follow-up patch on top of this fix (see attached) may not be a bad
thing to have, but that patch alone will not fix this issue without
dropping the unneeded and unwanted cast to unsigned long.
Thanks.
test-parse-options.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test-parse-options.c b/test-parse-options.c
index 434e8b8..6f6c656 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -60,7 +60,7 @@ int main(int argc, char **argv)
OPT_STRING('o', NULL, string, str, get another string),
OPT_NOOP_NOARG(0, obsolete),
OPT_SET_PTR(0, default-string, string,
- set string to default, (unsigned long)default),
+ set string to default, (intptr_t)default),
OPT_STRING_LIST(0, list, list, str, add str to list),
OPT_GROUP(Magic arguments),
OPT_ARGUMENT(quux, means --quux),
parse-options.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/parse-options.h b/parse-options.h
index d670cb9..7a24d2e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -129,7 +129,7 @@ struct option {
#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
NULL, 1}
#define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG, NULL, (p) }
+ (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p)
}
#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG|PARSE_OPT_NONEG,
NULL, (i) }
#define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(n),
(h) }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git feature request: Option to force Git to abort a checkout if working directory is dirty (i.e. disregarding the check for conflicts)
Jonas Bang em...@jonasbang.dk writes: For some people it is also a norm to keep files that have been modified from HEAD and/or index without committing for a long time (e.g. earlier, Linus said that the version in Makefile is updated and kept modified in the working tree long before a new release is committed with that change). The default behaviour would cover their use case so your proposal would not hurt them, but I wonder if there are things you could do to help them as well, perhaps by allowing this new configuration to express something like local changes in these paths are except from this new check. Yes, those people would probably use the default 'false' behavior as it is already. If they however would like to use e.g. the 'true' or 'include-untracked' setting as a configuration variable, then they can use the command line option 'false' if they wish to do a 'git checkout' even with modified files in the working tree. So in short, you are saying that The added code necessary to implement this feature will not help them in any way, it is just that we will make sure it does not hurt them. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] test-lib: '--run' to run only specific tests
Eric Sunshine sunsh...@sunshineco.com writes: Since the relational operators are fairly self-explanatory, you could drop the prose explanation, though that might make it too cryptic: A number prefixed with '', '=', '', or '=' matches test numbers meeting the specified relation. I would have to say that there is already an established pattern to pick ranges that normal people understand well and it would be silly to invent another more verbose way to express the same thing. You tell your Print Dialog which page to print with e.g. -4,7,9-12,15-, not =4 7 Would the same notation be insufficient for our purpose? You do not even have to worry about negation that way. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] commit: add --ignore-submodules[=when] parameter
Jens Lehmann jens.lehm...@web.de writes: Changes in add.c and cache.h (and related compilo fix in checkout.c) are needed to make it work for commit -a too. Looking good so far, but we definitely need tests for this new option. But I wonder if it would make more sense to start by teaching the --ignore-submodules option to git add. Then this patch could reuse that for commit -a. I am not sure about the code reuse, but from the usability and consistency point of view, they must go hand in hand, 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 v4 2/3] parse-options: add cast to correct pointer type to OPT_SET_PTR
Marat Radchenko ma...@slonopotamus.org writes:
Do not force users of OPT_SET_PTR to cast pointer to correct
underlying pointer type by integrating cast into OPT_SET_PTR macro.
Cast is required to prevent 'initialization makes integer from pointer
without a cast' compiler warning.
---
Signed-off-by (and probably From: too): Junio C Hamano gits...@pobox.com
;-)
parse-options.h | 2 +-
test-parse-options.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/parse-options.h b/parse-options.h
index 8fa02dc..54099d9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -129,7 +129,7 @@ struct option {
#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
NULL, 1}
#define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG, NULL, (p) }
+ (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p)
}
#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG|PARSE_OPT_NONEG,
NULL, (i) }
#define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v),
N_(n), (h) }
diff --git a/test-parse-options.c b/test-parse-options.c
index 6f6c656..10da63e 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -60,7 +60,7 @@ int main(int argc, char **argv)
OPT_STRING('o', NULL, string, str, get another string),
OPT_NOOP_NOARG(0, obsolete),
OPT_SET_PTR(0, default-string, string,
- set string to default, (intptr_t)default),
+ set string to default, default),
OPT_STRING_LIST(0, list, list, str, add str to list),
OPT_GROUP(Magic arguments),
OPT_ARGUMENT(quux, means --quux),
--
To unsubscribe from this list: send the line 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 0/3] Take four on fixing OPT_SET_PTR issues
Marat Radchenko ma...@slonopotamus.org writes: Patches summary: 1. Fix initial issue (incorrect cast causing crash on 64-bit MSVC) 2. Improve OPT_SET_PTR to prevent same errors in future 3. Purge OPT_SET_PTR away since nobody uses it *Optional* patch №3 is separated from №1 and №2 so that if someone someday decides to return OPT_SET_PTR back by reverting №3, it will be returned in a sane state. Decision of (not) merging №3 is left as an exercise to the reader due to my insufficient knowledge of accepted practices in Git project. SET_PTR() may not be used, but are there places where SET_INT() is abused with a cast-to-pointer for the same effect? I didn't check, but if there are such places, converting them to use SET_PTR() with their existing cast removed may be a better way to go. My suspicion is that there would be none, as switching the behaviour based on a small integer flag value is far easier than swapping the pointer to a pointee to be operated on, when responding to a command line option. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Don't rely on strerror text when testing rmdir failure
Jens Lehmann jens.lehm...@web.de writes: Am 29.03.2014 16:39, schrieb Charles Bailey: AIX doesn't make a distiction between EEXIST and ENOTEMPTY so relying on the strerror string for the rmdir failure is fragile. Just test that the start of the string matches the Git controlled failed to rmdir... error. The exact text of the OS generated error string isn't important to the test. Makes sense. Signed-off-by: Charles Bailey cbaile...@bloomberg.net --- t/t3600-rm.sh | 5 ++--- t/t7001-mv.sh | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 3d30581..23eed17 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -709,10 +709,9 @@ test_expect_success 'checking out a commit after submodule removal needs manual git commit -m submodule removal submod git checkout HEAD^ git submodule update -git checkout -q HEAD^ 2actual +git checkout -q HEAD^ 2/dev/null Isn't this unrelated to the strerror issue you are fixing here? Why not just drop the redirection completely? But maybe I'm just being to pedantic here ;-) No, that sounds like a very reasonable suggestion. Especially given that the redirection destination is overwritten immediately after. In general tests should not have to squelch their standard error output with 2/dev/null; that is a job for the test harness, and they will be shown in the output of ./t3600-rm -v to serve as anchor point while finding where a test goes wrong, which is a good thing. -- To unsubscribe from this list: send the line 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 report: Git 1.8 on Ubuntu 13.10 refs not valid
Jeff King p...@peff.net writes: That being said, git _could_ be more liberal in accepting a content-type with parameters (even though it does not know about any parameters, and charset here is completely meaningless). I have mixed feelings on that. It may be just a matter of replacing strbuf_cmp() with the initial part must be this string followed by it could have an optional whitespaces and semicolon after that, but I share the mixed feelings. I am not sure if it is a right thing to follow be liberal to accept dictum in this case. It may be liberal in accepting blindly, but if the other end is asking a behaviour that is not standard (but perhaps in future versions of Git such an enhanced service may be implemented by the client), by ignoring the parameter we do not even know about how to handle, we would be giving surprises to the overall protocol exchange. -- To unsubscribe from this list: send the line 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: Odd git diff breakage
Linus Torvalds torva...@linux-foundation.org writes: I hit this oddity when not remembering the right syntax for --color-words.. Try this (outside of a git repository): touch a b git diff -u --color=words a b and watch it scroll (infinitely) printing out error: option `color' expects always, auto, or never forever. I haven't tried to root-cause it, since I'm supposed to be merging stuff.. Linus Hmph, interesting. outside a repository is the key, it seems. And I can see it all the way down to 1.7.3 (at least). -- To unsubscribe from this list: send the line 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: Odd git diff breakage
Junio C Hamano gits...@pobox.com writes:
Linus Torvalds torva...@linux-foundation.org writes:
I hit this oddity when not remembering the right syntax for --color-words..
Try this (outside of a git repository):
touch a b
git diff -u --color=words a b
and watch it scroll (infinitely) printing out
error: option `color' expects always, auto, or never
forever.
I haven't tried to root-cause it, since I'm supposed to be merging stuff..
Linus
Hmph, interesting. outside a repository is the key, it seems.
And I can see it all the way down to 1.7.3 (at least).
diff_opt_parse() returns the number of options parsed, or often
returns error() which is defined to return -1. Yes, return value
of 0 is I don't understand that option, which should cause the
caller to say that, but negative return should not be forgotten.
diff-no-index.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index c554691..265709b 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -198,7 +198,7 @@ void diff_no_index(struct rev_info *revs,
i++;
else {
j = diff_opt_parse(revs-diffopt, argv + i, argc - i);
- if (!j)
+ if (j = 0)
die(invalid diff option/value: %s, argv[i]);
i += j;
}
--
To unsubscribe from this list: send the line 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-no-index: correctly diagnose error return from diff_opt_parse()
diff_opt_parse() returns the number of options parsed, or often
returns error() which is defined to return -1. Yes, return value of
0 is I did not process that option at all, which should cause the
caller to say that, but negative return should not be forgotten.
This bug the caused it to infinitely show the same error message
because the returned value was used to decrement the loop control
variable, e.g.
$ git diff --no-index --color=words a b
error: option `color' expects always, auto, or never
error: option `color' expects always, auto, or never
...
Instead, make it act like so:
$ git diff --no-index --color=words a b
error: option `color' expects always, auto, or never
fatal: invalid diff option/value: --color=words
Reported-by: Linus Torvalds torva...@linux-foundation.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* The other callchain to this function comes from rev-list.c, and
the caller does handle it correctly.
diff-no-index.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 00a8eef..ecf9293 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -244,7 +244,7 @@ void diff_no_index(struct rev_info *revs,
i++;
else {
j = diff_opt_parse(revs-diffopt, argv + i, argc - i);
- if (!j)
+ if (j = 0)
die(invalid diff option/value: %s, argv[i]);
i += j;
}
--
1.9.1-513-gce53123
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] patch-id: document new behaviour
Michael S. Tsirkin m...@redhat.com writes: Clarify that patch ID is now a sum of hashes, not a hash. Document --stable and --unstable flags. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- changes from v2: explicitly list the kinds of changes against which patch ID is stable Documentation/git-patch-id.txt | 23 ++- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt index 312c3b1..30923e0 100644 --- a/Documentation/git-patch-id.txt +++ b/Documentation/git-patch-id.txt @@ -8,14 +8,14 @@ git-patch-id - Compute unique ID for a patch SYNOPSIS [verse] -'git patch-id' patch +'git patch-id' [--stable | --unstable] patch Thanks. It seems taht we are fairly inconsistent when writing alternatives on the SYNOPSIS line. A small minority seems to spell the above as [--stable|--unstable], which may want to be fixed (outside the context of this series, of course). DESCRIPTION --- -A patch ID is nothing but a SHA-1 of the diff associated with a patch, with -whitespace and line numbers ignored. As such, it's reasonably stable, but at -the same time also reasonably unique, i.e., two patches that have the same patch -ID are almost guaranteed to be the same thing. +A patch ID is nothing but a sum of SHA-1 of the diff hunks associated with a +patch, with whitespace and line numbers ignored. As such, it's reasonably +stable, but at the same time also reasonably unique, i.e., two patches that +have the same patch ID are almost guaranteed to be the same thing. Perhaps nothing but can go by now? IOW, you can use this thing to look for likely duplicate commits. @@ -27,6 +27,19 @@ This can be used to make a mapping from patch ID to commit ID. OPTIONS --- + +--stable:: + Use a symmetrical sum of hashes as the patch ID. + With this option, reordering file diffs that make up a patch or + splitting a diff up to multiple diffs that touch the same path + does not affect the ID. + This is the default. + +--unstable:: + Use a non-symmetrical sum of hashes, such that reordering + or splitting the patch does affect the ID. + This was the default value for git 1.9 and older. I am not sure if swapping the default in this series is a wise decision. We typically introduce a new shiny toy to play with in a release and then later when the shiny toy proves to be useful, start to think about changing the default, but not before. patch:: The diff to create the ID of. -- To unsubscribe from this list: send the line 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 3/3] patch-id-test: test --stable and --unstable flags
Michael S. Tsirkin m...@redhat.com writes:
Verify that patch ID is now stable against diff split and reordering.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
Changes from v2:
added test to verify patch ID is stable against diff splitting
t/t4204-patch-id.sh | 117
1 file changed, 109 insertions(+), 8 deletions(-)
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index d2c930d..1679714 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -5,12 +5,46 @@ test_description='git patch-id'
. ./test-lib.sh
test_expect_success 'setup' '
- test_commit initial foo a
- test_commit first foo b
- git checkout -b same HEAD^
- test_commit same-msg foo b
- git checkout -b notsame HEAD^
- test_commit notsame-msg foo c
+ cat a -\EOF
Please do not add an extra space between the redirection operator
(e.g. , 2, etc.) and its operand. The same style violations
everywhere in this patch.
+ a
+ a
+ a
+ a
+ a
+ a
+ a
+ a
+ EOF
Please align EOF with the cat that receives it, and pretend that the
column the head of the cat is at is the leftmost column for the
payload, when writing - here-document, e.g.
cat a -\EOF
a
EOF
+ (cat a; echo b) ab
+ (echo d; cat a; echo b) dab
+ cp a foo
+ cp a bar
Probably a helper function would make it more apparent what is going
on?
as=a a a a a a a a # eight a
test_write_lines $as b ab
test_write_lines d $as b dab
test_write_lines $as foo
test_write_lines $as bar
Or even better, use test_write_lines in places you do use the result
to overwrite foo/bar and get rid of ab and dab?
That helper can also be used to prepare bar-then-foo.
test_expect_success 'patch-id output is well-formed' '
@@ -23,11 +57,33 @@ calc_patch_id () {
sed s# .*## patch-id_$1
}
+calc_patch_id_unstable () {
+ git patch-id --unstable |
+ sed s# .*## patch-id_$1
Not a new problem, but can you align git patch-id and sed to the
same column? Also, not using / when there is no slash involved in
the pattern makes readers waste their time wondering why the script
avoids it.
+}
+
+calc_patch_id_stable () {
+ git patch-id --stable |
+ sed s# .*## patch-id_$1
+}
+
+
Extra blank line.
get_patch_id () {
- git log -p -1 $1 | git patch-id |
+ git log -p -1 $1 -O bar-then-foo -- | git patch-id |
sed s# .*## patch-id_$1
}
+get_patch_id_stable () {
+ git log -p -1 $1 -O bar-then-foo | git patch-id --stable |
+ sed s# .*## patch-id_$1
Why doesn't it use calc_patch_id_stable?
+}
+
+get_patch_id_unstable () {
+ git log -p -1 $1 -O bar-then-foo | git patch-id --unstable |
+ sed s# .*## patch-id_$1
Ditto.
+}
+
+
Extra blank line.
test_expect_success 'patch-id detects equality' '
get_patch_id master
get_patch_id same
@@ -52,10 +108,55 @@ test_expect_success 'patch-id supports git-format-patch
output' '
test_expect_success 'whitespace is irrelevant in footer' '
get_patch_id master
git checkout same
- git format-patch -1 --stdout | sed s/ \$// | calc_patch_id same
+ git format-patch -1 --stdout | sed s/ \$// |
+ calc_patch_id same
What is this change about?
--
To unsubscribe from this list: send the line 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/3] patch-id: make it stable against hunk reordering
Michael S. Tsirkin m...@redhat.com writes: though it does look like an unrelated enhancement to me. Agree? Yes, that is exactly why I said opens interesting opportunity and making it possible ;-) They are all very related, but they do not have to graduate as parts of the same series. The important thing is that the basic infrastructure of the new codepath is designed in such a way that it later allows us to reuse it in these changes. I'm queuing these three patches almost as-is (I think I tweaked the sizeof thing) on 'pu', without fixing the test styles and other issues I may have pointed out in my reviews myself, expecting further clean-ups. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] patch-id: document new behaviour
Michael S. Tsirkin m...@redhat.com writes: The hash used is mostly an internal implementation detail, isn't it? Yes, but that does not mean we can break people who keep an external database indexed with the patch-id by changing the default under them, and they can give --unstable option to work it around is a workaround, not a fix. Without this change, they did not have to do anything. I would imagine that most of these people will be using the plain vanilla git show output without any ordering or hunk splitting when coming up with such a key. A possible way forward to allow the configuration that corresponds to -Oorderfile while not breaking the existing users could be to make the patch-id --stable kick in automatically (of course, do this only when the user did not give the --unstable command line option to override) when we see the orderfile configuration in the repository, or when we see that the incoming patch looks like reordered (e.g. has multiple diff --git header lines that refer to the same path, or the set of files mentioned by the diff --git lines are not in ascending order), perhaps? -- To unsubscribe from this list: send the line 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: [RFC] submodule: change submodule.name.branch default from master to HEAD
Jens Lehmann jens.lehm...@web.de writes: I'd prefer a solution that doesn't change any defaults for the checkout use case (again). Maybe it is a better route to revert this series, then add tests describing the current behavior for checkout submodules as a next step before adding the branch mode for rebase and merge users again? Sounds like a good idea to revert the entire series, as that would give us longer to think the whole thing through. It would certainly avoid unexpected fallout in 2.0 where we already have other changes, all of which are known/designed to be backward incompatible with a set of carefully planned transition plans. This submodule change may have meant well, but does not seem to sit well together with these other changes. Perhaps in 2.1 or 2.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: socket_perror() bug?
Thiago Farina tfrans...@gmail.com writes: In imap-send.c:socket_perror() we pass |func| as a parameter, which I think it is the name of the function that called socket_perror, or the name of the function which generated an error. But at line 184 and 187 it always assume it was SSL_connect. Should we instead call perror() and ssl_socket_error() with func? Looks that way to me, at least from a cursory look. -- To unsubscribe from this list: send the line 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 02/27] t1400: Provide more usual input to the command
Michael Haggerty mhag...@alum.mit.edu writes:
Subject: Re: [PATCH v2 02/27] t1400: Provide more usual input to the command
This applies to the patches throughout the series, but during the
microproject reviews, Eric pointed out that we seem to start the
summary after area: on the subject with lowercase and omit the full
stop at the end, so I'll try to tweak the subjects while queueing to
read patches in the series.
The old version was passing (among other things)
update SP refs/heads/c NUL NUL 0{40} NUL
to git update-ref -z --stdin to test whether the old-value check for
c is working. But the newvalue is empty, which is a bit off the
beaten track.
So, to be sure that we are testing what we want to test, provide an
actual newvalue on the update line.
Interesting. So the test used to expect failure, but we couldn't
tell if that was due to giving a Please update to this value which
is malformed, or I am giving 0{40} as the old value, telling you
that you have to make sure the ref does not exist which does not
hold because we already have that ref?
That would mean that the test may not have been testing the right
thing. A good change.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
t/t1400-update-ref.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index fa927d2..29391c6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -912,7 +912,7 @@ test_expect_success 'stdin -z update refs works with
identity updates' '
test_expect_success 'stdin -z update refs fails with wrong old value' '
git update-ref $c $m
- printf $F update $a $m $m update $b $m $m update $c
$Z stdin
+ printf $F update $a $m $m update $b $m $m update $c $m
$Z stdin
test_must_fail git update-ref -z --stdin stdin 2err
grep fatal: Cannot lock the ref '''$c''' err
git rev-parse $m expect
--
To unsubscribe from this list: send the line 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 01/27] t1400: Fix name and expected result of one test
Michael Haggerty mhag...@alum.mit.edu writes: The test stdin -z create ref fails with zero new value actually passes an empty new value, not a zero new value. So rename the test s/zero/empty/, and change the expected error from fatal: create $c given zero new value to fatal: create $c missing newvalue I have a feeling that zero new value might have been done by a non-native (like me) to say no new value; missing newvalue sounds like a good phrasing to use. Of course, this makes the test fail now, so mark it test_expect_failure. The failure will be fixed later in this patch series. That sounds somewhat strange. Why not just give a single-liner to update-ref.c instead? Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- t/t1400-update-ref.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 6ffd82f..fa927d2 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -827,10 +827,10 @@ test_expect_success 'stdin -z create ref fails with bad new value' ' test_must_fail git rev-parse --verify -q $c ' -test_expect_success 'stdin -z create ref fails with zero new value' ' +test_expect_failure 'stdin -z create ref fails with empty new value' ' printf $F create $c stdin test_must_fail git update-ref -z --stdin stdin 2err - grep fatal: create $c given zero new value err + grep fatal: create $c missing newvalue err test_must_fail git rev-parse --verify -q $c ' -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/27] parse_arg(): Really test that argument is properly terminated
Michael Haggerty mhag...@alum.mit.edu writes:
Test that the argument is properly terminated by either whitespace or
a NUL character, even if it is quoted, to be consistent with the
non-quoted case. Adjust the tests to expect the new error message.
Add a docstring to the function, incorporating the comments that were
formerly within the function plus some added information.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/update-ref.c | 20 +++-
t/t1400-update-ref.sh | 4 ++--
2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1292cfe..02b5f95 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update
*update,
update-have_old = *oldvalue || line_termination;
}
+/*
+ * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
+ * and append the result to arg. Return a pointer to the terminator.
+ * Die if there is an error in how the argument is C-quoted. This
+ * function is only used if not -z.
+ */
static const char *parse_arg(const char *next, struct strbuf *arg)
{
- /* Parse SP-terminated, possibly C-quoted argument */
- if (*next != '')
+ if (*next == '') {
+ const char *orig = next;
+
+ if (unquote_c_style(arg, next, next))
+ die(badly quoted argument: %s, orig);
+ if (*next !isspace(*next))
+ die(unexpected character after quoted argument: %s,
orig);
+ } else {
while (*next !isspace(*next))
strbuf_addch(arg, *next++);
- else if (unquote_c_style(arg, next, next))
- die(badly quoted argument: %s, next);
+ }
- /* Return position after the argument */
return next;
}
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 29391c6..774f8c5 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input'
'
grep fatal: badly quoted argument: \\\master err
'
-test_expect_success 'stdin fails on arguments not separated by space' '
+test_expect_success 'stdin fails on junk after quoted argument' '
echo create \$a\master stdin
test_must_fail git update-ref --stdin stdin 2err
- grep fatal: expected SP but got: master err
+ grep fatal: unexpected character after quoted argument:
\\\$a\\\master err
'
Interesting.
I would have expected that We used to check only one of the two
codepaths and the other was loose, fix it to be accompanied by So
here is an _addition_ to the test suite to validate the other case
that used to be loose, now tightened, not We update one existing
case. The test before and after the patch is about a c-quoted
string, so I am not sure if we are still testing the right thing.
The code in update-ref.c after the patch does look reasonable,
though.
--
To unsubscribe from this list: send the line 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 06/27] update_refs(): Fix constness
Michael Haggerty mhag...@alum.mit.edu writes:
Since full const correctness is beyond the ability of C's type system,
just put the const where it doesn't do any harm. A (struct ref_update
**) can be passed to a (struct ref_update * const *) argument, but not
to a (const struct ref_update **) argument.
Sounds good, but next time please try not to break lines inside a
single typename, which is somewhat unreadable ;-)
I'd suggest rewording s/Fix/tighten/. Because a patch that
changes constness can loosen constness to make things more correct,
git shortlog output that says if it is tightening or loosening
would be more informative than the one that says that it is fixing.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/update-ref.c | 2 +-
refs.c | 2 +-
refs.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index f6345e5..a8a68e8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -14,7 +14,7 @@ static const char * const git_update_ref_usage[] = {
static int updates_alloc;
static int updates_count;
-static const struct ref_update **updates;
+static struct ref_update **updates;
static char line_termination = '\n';
static int update_flags;
diff --git a/refs.c b/refs.c
index 196984e..1305eb1 100644
--- a/refs.c
+++ b/refs.c
@@ -3306,7 +3306,7 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
return 0;
}
-int update_refs(const char *action, const struct ref_update **updates_orig,
+int update_refs(const char *action, struct ref_update * const *updates_orig,
int n, enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
diff --git a/refs.h b/refs.h
index a713b34..08e60ac 100644
--- a/refs.h
+++ b/refs.h
@@ -228,7 +228,7 @@ int update_ref(const char *action, const char *refname,
/**
* Lock all refs and then perform all modifications.
*/
-int update_refs(const char *action, const struct ref_update **updates,
+int update_refs(const char *action, struct ref_update * const *updates,
int n, enum action_on_err onerr);
extern int parse_hide_refs_config(const char *var, const char *value, const
char *);
--
To unsubscribe from this list: send the line 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 13/27] t1400: Test that stdin -z update treats empty newvalue as zeros
Michael Haggerty mhag...@alum.mit.edu writes:
This is the (slightly inconsistent) status quo; make sure it doesn't
change by accident.
Interesting. So oldvalue being empty is we do not care what it
is (as opposed to we know it must not exist yet aka 0{40}), and
newvalue being empty is the same as delete it aka 0{40}.
That is unfortunate, but I agree it is a good idea to add a test for
it, so that we will notice when we decide to fix it.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
t/t1400-update-ref.sh | 7 +++
1 file changed, 7 insertions(+)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index a2015d0..208f56e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -730,6 +730,13 @@ test_expect_success 'stdin -z fails update with bad ref
name' '
grep fatal: invalid ref format: ~a err
'
+test_expect_success 'stdin -z treats empty new value as zeros' '
+ git update-ref $a $m
+ printf $F update $a stdin
+ git update-ref -z --stdin stdin
+ test_must_fail git rev-parse --verify -q $a
+'
+
test_expect_success 'stdin -z fails update with no new value' '
printf $F update $a stdin
test_must_fail git update-ref -z --stdin stdin 2err
--
To unsubscribe from this list: send the line 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 15/27] update-ref --stdin -z: Deprecate interpreting the empty string as zeros
Michael Haggerty mhag...@alum.mit.edu writes:
In the original version of this command, for the single case of the
update command's newvalue, the empty string was interpreted as
being equivalent to 40 0s. This shorthand is unnecessary (binary
input will usually be generated programmatically anyway), and it
complicates the parser and the documentation.
Nice.
So gently deprecate this usage: remove its description from the
documentation and emit a warning if it is found. But for reasons of
backwards compatibility, continue to accept it.
Helped-by: Brad King brad.k...@kitware.com
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
Documentation/git-update-ref.txt | 18 --
builtin/update-ref.c | 2 ++
t/t1400-update-ref.sh| 5 +++--
3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-update-ref.txt
b/Documentation/git-update-ref.txt
index 0a0a551..c8f5ae5 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -68,7 +68,12 @@ performs all modifications together. Specify commands of
the form:
option SP opt LF
Quote fields containing whitespace as if they were strings in C source
-code. Alternatively, use `-z` to specify commands without quoting:
+code; i.e., surrounded by double-quotes and with backslash escapes.
+Use 40 0 characters or the empty string to specify a zero value. To
+specify a missing value, omit the value and its preceding SP entirely.
+
+Alternatively, use `-z` to specify in NUL-terminated format, without
+quoting:
update SP ref NUL newvalue NUL [oldvalue] NUL
create SP ref NUL newvalue NUL
@@ -76,8 +81,12 @@ code. Alternatively, use `-z` to specify commands without
quoting:
verify SP ref NUL [oldvalue] NUL
option SP opt NUL
-Lines of any other format or a repeated ref produce an error.
-Command meanings are:
+In this format, use 40 0 to specify a zero value, and use the empty
+string to specify a missing value.
+
+In either format, values can be specified in any form that Git
+recognizes as an object name. Commands in any other format or a
+repeated ref produce an error. Command meanings are:
update::
Set ref to newvalue after verifying oldvalue, if given.
@@ -102,9 +111,6 @@ option::
The only valid option is `no-deref` to avoid dereferencing
a symbolic ref.
-Use 40 0 or the empty string to specify a zero value, except that
-with `-z` an empty oldvalue is considered missing.
-
If all refs can be locked with matching oldvalues
simultaneously, all modifications are performed. Otherwise, no
modifications are performed. Note that while each individual
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6462b2f..eef7537 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -154,6 +154,8 @@ static int parse_next_sha1(struct strbuf *input, const
char **next,
goto invalid;
} else if (flags PARSE_SHA1_ALLOW_EMPTY) {
/* With -z, treat an empty value as all zeros: */
+ warning(%s %s: missing newvalue, treating as zero,
+ command, refname);
hashclr(sha1);
} else {
/*
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 15f5bfd..2d61cce 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -730,10 +730,11 @@ test_expect_success 'stdin -z fails update with bad ref
name' '
grep fatal: invalid ref format: ~a err
'
-test_expect_success 'stdin -z treats empty new value as zeros' '
+test_expect_success 'stdin -z emits warning with empty new value' '
git update-ref $a $m
printf $F update $a stdin
- git update-ref -z --stdin stdin
+ git update-ref -z --stdin stdin 2err
+ grep warning: update $a: missing newvalue, treating as zero err
test_must_fail git rev-parse --verify -q $a
'
--
To unsubscribe from this list: send the line 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 18/27] update-ref --stdin: Harmonize error messages
Michael Haggerty mhag...@alum.mit.edu writes:
Make (most of) the error messages for invalid input have the same
format [1]:
$COMMAND [SP $REFNAME]: $MESSAGE
Update the tests accordingly.
[1] A few error messages are left with their old form, because
$COMMAND and $REFNAME aren't passed all the way down the call
stack. Maybe those sites should be changed some day, too.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
Up to this point, modulo nits that have been pointed out separately,
the series looked reasonably well done.
Thanks.
builtin/update-ref.c | 24
t/t1400-update-ref.sh | 32
2 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index b49a5b0..bbc04b2 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -202,19 +202,19 @@ static const char *parse_cmd_update(struct strbuf
*input, const char *next)
update-ref_name = parse_refname(input, next);
if (!update-ref_name)
- die(update line missing ref);
+ die(update: missing ref);
if (parse_next_sha1(input, next, update-new_sha1,
update, update-ref_name,
PARSE_SHA1_ALLOW_EMPTY))
- die(update %s missing newvalue, update-ref_name);
+ die(update %s: missing newvalue, update-ref_name);
update-have_old = !parse_next_sha1(input, next, update-old_sha1,
update, update-ref_name,
PARSE_SHA1_OLD);
if (*next != line_termination)
- die(update %s has extra input: %s, update-ref_name, next);
+ die(update %s: extra input: %s, update-ref_name, next);
return next;
}
@@ -227,17 +227,17 @@ static const char *parse_cmd_create(struct strbuf
*input, const char *next)
update-ref_name = parse_refname(input, next);
if (!update-ref_name)
- die(create line missing ref);
+ die(create: missing ref);
if (parse_next_sha1(input, next, update-new_sha1,
create, update-ref_name, 0))
- die(create %s missing newvalue, update-ref_name);
+ die(create %s: missing newvalue, update-ref_name);
if (is_null_sha1(update-new_sha1))
- die(create %s given zero newvalue, update-ref_name);
+ die(create %s: zero newvalue, update-ref_name);
if (*next != line_termination)
- die(create %s has extra input: %s, update-ref_name, next);
+ die(create %s: extra input: %s, update-ref_name, next);
return next;
}
@@ -250,19 +250,19 @@ static const char *parse_cmd_delete(struct strbuf
*input, const char *next)
update-ref_name = parse_refname(input, next);
if (!update-ref_name)
- die(delete line missing ref);
+ die(delete: missing ref);
if (parse_next_sha1(input, next, update-old_sha1,
delete, update-ref_name, PARSE_SHA1_OLD)) {
update-have_old = 0;
} else {
if (is_null_sha1(update-old_sha1))
- die(delete %s given zero oldvalue,
update-ref_name);
+ die(delete %s: zero oldvalue, update-ref_name);
update-have_old = 1;
}
if (*next != line_termination)
- die(delete %s has extra input: %s, update-ref_name, next);
+ die(delete %s: extra input: %s, update-ref_name, next);
return next;
}
@@ -275,7 +275,7 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
update-ref_name = parse_refname(input, next);
if (!update-ref_name)
- die(verify line missing ref);
+ die(verify: missing ref);
if (parse_next_sha1(input, next, update-old_sha1,
verify, update-ref_name, PARSE_SHA1_OLD)) {
@@ -286,7 +286,7 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
}
if (*next != line_termination)
- die(verify %s has extra input: %s, update-ref_name, next);
+ die(verify %s: extra input: %s, update-ref_name, next);
return next;
}
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 1db0689..48ccc4d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -371,7 +371,7 @@ test_expect_success 'stdin fails on junk after quoted
argument' '
test_expect_success 'stdin fails create with no ref' '
echo create stdin
test_must_fail git update-ref --stdin stdin 2err
- grep fatal: create line missing ref err
+ grep fatal: create: missing ref err
'
test_expect_success 'stdin fails create with bad ref name' '
@@ -383,19 +383,19 @@ test_expect_success
Re: [PATCH 3/4] Fix misuses of nor in comments. (v3)
Justin Lebar jle...@google.com writes:
Thanks; fixed in v4 (just sent out).
I only saw [3/4] that was marked with (v3) at the end, without
[{1,2,4}/4]. As you seem to be renumbering from the very original
(which had l10n at number 3), only sending out what you changed,
expecting that everybody else knows what you are doing, is not very
friendly to anybody who is trying not to lose patches in all the
other activities on the list. Also people who haven't seen the
older iterations do not have to fish them from the archive if you
repost the whole thing.
Please use format-patch -v5 (or whatever numbering) so that the
series iteration number is inside, e.g. [PATCH v5 1/6].
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, #07; Fri, 28)
Duy Nguyen pclo...@gmail.com writes: On Sat, Mar 29, 2014 at 5:21 AM, Junio C Hamano gits...@pobox.com wrote: * nd/gc-aggressive (2014-03-17) 4 commits - gc --aggressive: three phase repacking - gc --aggressive: make --depth configurable - pack-objects: support --keep - environment.c: fix constness for odb_pack_keep() Using --before=1.year.ago my way does not keep all recent chains short because commit time is unreliable. But even with better recent object selection, the performance gain is still within noise level. I suggest we keep gc --aggressive: make --depth configurable and drop the rest. I think the three-phase thing may not be so well conceived or cooked, but the other three looked like reasonable changes, although it would have been easier to say so if pack-objects --keep were done without a new buffer of size PATH_MAX ;-) Let's do the const and --depth and discard the other two. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
