[PATCH] git-p4: correct --prepare-p4-only instructions
If you use git-p4 with the "--prepare-p4-only" option, then it prints the p4 command line to use. However, the command line was incorrect: the changelist specification must be supplied on standard input, not as an argument to p4. Signed-off-by: Luke Diamand --- git-p4.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-p4.py b/git-p4.py index ff132b2..90447de 100755 --- a/git-p4.py +++ b/git-p4.py @@ -1442,7 +1442,7 @@ class P4Submit(Command, P4UserMap): print " " + self.clientPath print print "To submit, use \"p4 submit\" to write a new description," -print "or \"p4 submit -i %s\" to use the one prepared by" \ +print "or \"p4 submit -i <%s\" to use the one prepared by" \ " \"git p4\"." % fileName print "You can delete the file \"%s\" when finished." % fileName -- 2.1.3.1037.g95a6691.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-p4: correct --prepare-p4-only instructions
This fixes a small error in the command that git-p4 suggests when the user gives the --prepare-p4-only option. It tells the user to use "p4 submit -i filename" but the p4 submit command reads a change specification on standard input. The correct command line is therefore: p4 submit -i http://vger.kernel.org/majordomo-info.html
Re: Should copy/rename detection consider file overwrites?
On Fri, Jan 23, 2015 at 10:29:08AM +0900, Mike Hommey wrote: > While fooling around with copy/rename detection, I noticed that it > doesn't detect the case where you copy or rename a file on top of > another: > > $ git init > $ (echo foo; echo bar) > foo If I replace this with a longer input, like: cp /usr/share/dict/words foo > $ git add foo > $ git commit -m foo > $ echo 0 > bar > $ git add bar > $ git commit -m bar > $ git mv -f foo bar > $ git commit -m foobar > $ git log --oneline --reverse > 7dc2765 foo > b0c837d bar > 88caeba foobar > $ git blame -s -C -C bar > 88caebab 1) foo > 88caebab 2) bar Then the blame shows me the initial "foo" commit. So I think it is mainly that your toy example is too small (I think we will do exact rename detection whatever the size is, but I expect we are getting hung up on the break detection between "0\n" and "foo\nbar\n"). > I can see how this is not trivially representable in e.g. git diff-tree, > but shouldn't at least blame try to tell that those lines actually come > from 7dc2765? diff-tree can show this, too, but you need to turn on "break detection" which will notice that "bar" has essentially been rewritten (and then consider its sides as candidates for rename detection). For example (with the longer input, as above): $ git diff-tree --name-status -M HEAD c6fe146b0c73adcbc4dbc2e58eb83af9007678bc M bar D foo $ git diff-tree --name-status -M -B HEAD c6fe146b0c73adcbc4dbc2e58eb83af9007678bc R100foo bar Presumably if you set the break score low enough, your original example would behave the same way, but I couldn't get it to work (I didn't look closely, but I imagine it is just so tiny that we hit the internal limits on how low you can set the score). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] show-branch: minor style fix
Asterisk must be next with variable
Signed-off-by: Alexander Kuleshov
---
builtin/show-branch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 3a7ec55..e7684c8 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -6,7 +6,7 @@
#include "parse-options.h"
#include "commit-slab.h"
-static const char* show_branch_usage[] = {
+static const char *show_branch_usage[] = {
N_("git show-branch [-a | --all] [-r | --remotes] [--topo-order |
--date-order]\n"
" [--current] [--color[=] | --no-color]
[--sparse]\n"
" [--more= | --list | --independent | --merge-base]\n"
--
2.3.0.rc1.275.g028c360
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] Coding style fixes.
This patch set contatins minor style fixes. CodingGuidelines contains rule that the star must side with variable name. Alexander Kuleshov (7): show-branch: minor style fix clone: minor style fix test-hashmap: minor style fix http-backend: minor style fix refs: minor style fix quote: minor style fix fast-import: minor style fix builtin/clone.c | 2 +- builtin/show-branch.c | 2 +- fast-import.c | 2 +- http-backend.c| 2 +- quote.c | 2 +- refs.h| 2 +- test-hashmap.c| 4 ++-- 7 files changed, 8 insertions(+), 8 deletions(-) -- 2.3.0.rc1.275.g028c360 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/7] http-backend: minor style fix
Asterisk must be next with variable
Signed-off-by: Alexander Kuleshov
---
http-backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/http-backend.c b/http-backend.c
index b6c0484..7b5670b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -515,7 +515,7 @@ static NORETURN void die_webcgi(const char *err, va_list
params)
exit(0); /* we successfully reported a failure ;-) */
}
-static char* getdir(void)
+static char *getdir(void)
{
struct strbuf buf = STRBUF_INIT;
char *pathinfo = getenv("PATH_INFO");
--
2.3.0.rc1.275.g028c360
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] clone: minor style fix
Asterisk must be next with variable
Signed-off-by: Alexander Kuleshov
---
builtin/clone.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index d262a4d..a1ca780 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -741,7 +741,7 @@ static void write_refspec_config(const char *src_ref_prefix,
static void dissociate_from_references(void)
{
- static const char* argv[] = { "repack", "-a", "-d", NULL };
+ static const char *argv[] = { "repack", "-a", "-d", NULL };
if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
die(_("cannot repack to clean up"));
--
2.3.0.rc1.275.g028c360
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/7] refs: minor style fix
Asterisk must be next with variable Signed-off-by: Alexander Kuleshov --- refs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.h b/refs.h index 425ecf7..bd8afe2 100644 --- a/refs.h +++ b/refs.h @@ -86,7 +86,7 @@ extern int for_each_branch_ref(each_ref_fn, void *); extern int for_each_remote_ref(each_ref_fn, void *); extern int for_each_replace_ref(each_ref_fn, void *); extern int for_each_glob_ref(each_ref_fn, const char *pattern, void *); -extern int for_each_glob_ref_in(each_ref_fn, const char *pattern, const char* prefix, void *); +extern int for_each_glob_ref_in(each_ref_fn, const char *pattern, const char *prefix, void *); extern int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data); extern int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data); -- 2.3.0.rc1.275.g028c360 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/7] test-hashmap: minor style fix
Asterisk must be next with variable
Signed-off-by: Alexander Kuleshov
---
test-hashmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/test-hashmap.c b/test-hashmap.c
index cc2891d..5f67120 100644
--- a/test-hashmap.c
+++ b/test-hashmap.c
@@ -14,13 +14,13 @@ static const char *get_value(const struct test_entry *e)
}
static int test_entry_cmp(const struct test_entry *e1,
- const struct test_entry *e2, const char* key)
+ const struct test_entry *e2, const char *key)
{
return strcmp(e1->key, key ? key : e2->key);
}
static int test_entry_cmp_icase(const struct test_entry *e1,
- const struct test_entry *e2, const char* key)
+ const struct test_entry *e2, const char *key)
{
return strcasecmp(e1->key, key ? key : e2->key);
}
--
2.3.0.rc1.275.g028c360
--
To unsubscribe from this list: send the line "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 7/7] fast-import: minor style fix
Asterisk must be next with variable
Signed-off-by: Alexander Kuleshov
---
fast-import.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fast-import.c b/fast-import.c
index 1b50923..fec67ca 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3110,7 +3110,7 @@ static void parse_progress(void)
skip_optional_lf();
}
-static char* make_fast_import_path(const char *path)
+static char *make_fast_import_path(const char *path)
{
if (!relative_marks_paths || is_absolute_path(path))
return xstrdup(path);
--
2.3.0.rc1.275.g028c360
--
To unsubscribe from this list: send the line "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 6/7] quote: minor style fix
Asterisk must be next with variable
Signed-off-by: Alexander Kuleshov
---
quote.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/quote.c b/quote.c
index 7920e18..02e9a3c 100644
--- a/quote.c
+++ b/quote.c
@@ -42,7 +42,7 @@ void sq_quote_buf(struct strbuf *dst, const char *src)
free(to_free);
}
-void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
+void sq_quote_argv(struct strbuf *dst, const char **argv, size_t maxlen)
{
int i;
--
2.3.0.rc1.275.g028c360
--
To unsubscribe from this list: send the line "unsubscribe 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 compile warnings (under mac/clang)
Hi Peff,
On 2015-01-22 23:01, Jeff King wrote:
> On Thu, Jan 22, 2015 at 10:20:01PM +0100, Johannes Schindelin wrote:
>
>> On 2015-01-22 20:59, Stefan Beller wrote:
>> > cc Johannes Schindelin who is working in
>> > the fsck at the moment
>> >
>> > On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume
>> > wrote:
>> >
>> >> CC fsck.o
>> >> fsck.c:110:38: warning: comparison of unsigned enum expression >= 0 is
>> >> always true [-Wtautological-compare]
>> >> if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
>> >> ~~ ^ ~
>>
>> According to A2.5.4 of The C Programming Language 2nd edition:
>>
>> Identifiers declared as enumerators (see Par.A.8.4) are constants of
>> type int.
>>
>> Therefore, the warning is incorrect: any assumption about enum fsck_msg_id
>> to be unsigned is false.
>
> I'm not sure that made it to ANSI. C99 says (setion 6.7.2.2, paragraph
> 4):
>
> Each enumerated type shall be compatible with char, a signed integer
> type, or an unsigned integer type. The choice of type is
> implementation-defined, but shall be capable of representing the
> values of all the members of the enumeration.
>
> I don't have a copy of C89, but this isn't mentioned in the (very
> cursory) list of changes found in C99. Anyway, that's academic.
>
> I think we dealt with a similar situation before, in
> 3ce3ffb840a1dfa7fcbafa9309fab37478605d08.
Ww. That commit got a chuckle out of me...
This is what I have currently in the way of attempting to "fix" it (I still
believe that Clang is wrong to make this a warning, and causes more trouble
than it solves):
-- snipsnap --
commit 11b4c713f77081bf8342e5c02055ae8e18d28e8b
Author: Johannes Schindelin
Date: Fri Jan 23 12:46:02 2015 +0100
fsck: fix clang -Wtautological-compare with unsigned enum
Clang warns that the fsck_msg_id enum is unsigned, missing that the
specification of the C language allows for C compilers interpreting
enums as signed.
To shut up Clang, we waste a full enum value just so that we compare
against an enum value without messing up the readability of the source
code.
Pointed out by Michael Blume. Jeff King provided the pointer to a commit
fixing the same issue elsewhere in the Git source code.
Signed-off-by: Johannes Schindelin
diff --git a/fsck.c b/fsck.c
index 15cb8bd..f76e7a9 100644
--- a/fsck.c
+++ b/fsck.c
@@ -65,6 +65,7 @@
#define MSG_ID(id, severity) FSCK_MSG_##id,
enum fsck_msg_id {
+ FSCK_MSG_MIN = 0,
FOREACH_MSG_ID(MSG_ID)
FSCK_MSG_MAX
};
@@ -76,6 +77,7 @@ static struct {
const char *id_string;
int severity;
} msg_id_info[FSCK_MSG_MAX + 1] = {
+ { NULL, -1 },
FOREACH_MSG_ID(MSG_ID)
{ NULL, -1 }
};
@@ -85,7 +87,7 @@ static int parse_msg_id(const char *text, int len)
{
int i, j;
- for (i = 0; i < FSCK_MSG_MAX; i++) {
+ for (i = FSCK_MSG_MIN + 1; i < FSCK_MSG_MAX; i++) {
const char *key = msg_id_info[i].id_string;
/* id_string is upper-case, with underscores */
for (j = 0; j < len; j++) {
@@ -107,7 +109,8 @@ static int fsck_msg_severity(enum fsck_msg_id msg_id,
{
int severity;
- if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
+ if (options->msg_severity &&
+ msg_id > FSCK_MSG_MIN && msg_id < FSCK_MSG_MAX)
severity = options->msg_severity[msg_id];
else {
severity = msg_id_info[msg_id].severity;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Coding style fixes.
I made separate patch for every file. Please, let me know if need to squash it into one commit. 2015-01-23 17:06 GMT+06:00 Alexander Kuleshov : > This patch set contatins minor style fixes. CodingGuidelines contains > rule that the star must side with variable name. > > Alexander Kuleshov (7): > show-branch: minor style fix > clone: minor style fix > test-hashmap: minor style fix > http-backend: minor style fix > refs: minor style fix > quote: minor style fix > fast-import: minor style fix > > builtin/clone.c | 2 +- > builtin/show-branch.c | 2 +- > fast-import.c | 2 +- > http-backend.c| 2 +- > quote.c | 2 +- > refs.h| 2 +- > test-hashmap.c| 4 ++-- > 7 files changed, 8 insertions(+), 8 deletions(-) > > -- > 2.3.0.rc1.275.g028c360 -- _ 0xAX -- To unsubscribe from this list: send the line "unsubscribe 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 7/7] fast-import: minor style fix
On 2015-01-23 12.08, Alexander Kuleshov wrote: .. Asterisk must be next with variable .. But this is a function: > -static char* make_fast_import_path(const char *path) > +static char *make_fast_import_path(const char *path) (Sorry when I need to read this:) - Fixing style violations while working on a real change as a preparatory clean-up step is good, but otherwise avoid useless code churn for the sake of conforming to the style. "Once it _is_ in the tree, it's not really worth the patch noise to go and fix it up." Cf. http://article.gmane.org/gmane.linux.kernel/943020 -- To unsubscribe from this list: send the line "unsubscribe 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 compile warnings (under mac/clang)
On Fri, Jan 23, 2015 at 12:48:29PM +0100, Johannes Schindelin wrote:
> This is what I have currently in the way of attempting to "fix" it (I
> still believe that Clang is wrong to make this a warning, and causes
> more trouble than it solves):
I agree. It is something we as the programmers cannot possibly know (the
compiler is free to decide which type however it likes) and its decision
does not impact the correctness of the code (the check is either useful
or tautological, and we cannot know which, so we are being warned about
being too careful!).
I guess you could argue that the standard defines enum-numbering to
start at 0, and increment by 1. Therefore we should know that no valid
enum value is less than 0. IOW, "msg_id < 0" being true must be the
result of a bug somewhere else in the program, where we assigned a value
outside of the enum range to the enum.
> Pointed out by Michael Blume. Jeff King provided the pointer to a commit
> fixing the same issue elsewhere in the Git source code.
It may be useful to reference the exact commit (3ce3ffb8) to help people
digging in the history (e.g., if we decide there is a better way to shut
up this warning and we need to find all the places to undo the
brain-damage).
> - for (i = 0; i < FSCK_MSG_MAX; i++) {
> + for (i = FSCK_MSG_MIN + 1; i < FSCK_MSG_MAX; i++) {
Ugh. It is really a shame how covering up this warning requires
polluting so many places. I don't think we have a better way, though,
aside from telling people to use -Wno-tautological-compare (and I can
believe that it _is_ a useful warning in some other circumstances, so it
seems a shame to lose it).
Unless we are willing to drop the ">= 0" check completely. I think it is
valid to do so regardless of the compiler's representation decision due
to the numbering rules I mentioned above. It kind-of serves as a
cross-check that we haven't cast some random int into the enum, but I
think we would do better to find those callsites (since they are not
guaranteed to work, anyway; in addition to signedness, it might choose a
much smaller representation).
I do not see either side of the bounds check here:
> + if (options->msg_severity &&
> + msg_id > FSCK_MSG_MIN && msg_id < FSCK_MSG_MAX)
as really doing anything. Any code which triggers it must already cause
undefined behavior, I think (with the exception of "msg_id == FSCK_MSG_MAX",
but presumably that is something we never expect to happen, either).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git compile warnings (under mac/clang)
Hi Peff,
On 2015-01-23 13:23, Jeff King wrote:
> On Fri, Jan 23, 2015 at 12:48:29PM +0100, Johannes Schindelin wrote:
>
>> Pointed out by Michael Blume. Jeff King provided the pointer to a commit
>> fixing the same issue elsewhere in the Git source code.
>
> It may be useful to reference the exact commit (3ce3ffb8) to help people
> digging in the history (e.g., if we decide there is a better way to shut
> up this warning and we need to find all the places to undo the
> brain-damage).
Good point, thanks!
>> -for (i = 0; i < FSCK_MSG_MAX; i++) {
>> +for (i = FSCK_MSG_MIN + 1; i < FSCK_MSG_MAX; i++) {
>
> Ugh. It is really a shame how covering up this warning requires
> polluting so many places. I don't think we have a better way, though,
> aside from telling people to use -Wno-tautological-compare (and I can
> believe that it _is_ a useful warning in some other circumstances, so it
> seems a shame to lose it).
>
> Unless we are willing to drop the ">= 0" check completely. I think it is
> valid to do so regardless of the compiler's representation decision due
> to the numbering rules I mentioned above. It kind-of serves as a
> cross-check that we haven't cast some random int into the enum, but I
> think we would do better to find those callsites (since they are not
> guaranteed to work, anyway; in addition to signedness, it might choose a
> much smaller representation).
Yeah, well, this check is really more of a safety net in case I messed up
anything; I was saved so many times by my own defensive programming that I try
to employ it as much as I can.
But it does complicate the papering over Clang's overzealous warning, so I
could live with removing the check altogether.
On the other hand, I could do something even easier:
-- snip --
diff --git a/fsck.c b/fsck.c
index 15cb8bd..8f8c82f 100644
--- a/fsck.c
+++ b/fsck.c
@@ -107,7 +107,7 @@ static int fsck_msg_severity(enum fsck_msg_id msg_id,
{
int severity;
- if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
+ if (options->msg_severity && ((unsigned int) msg_id) < FSCK_MSG_MAX)
severity = options->msg_severity[msg_id];
else {
severity = msg_id_info[msg_id].severity;
-- snap --
What do you think? Michael, does this cause more Clang warnings, or would it
resolve the issue?
> I do not see either side of the bounds check here:
>
>> +if (options->msg_severity &&
>> +msg_id > FSCK_MSG_MIN && msg_id < FSCK_MSG_MAX)
>
> as really doing anything. Any code which triggers it must already cause
> undefined behavior, I think (with the exception of "msg_id == FSCK_MSG_MAX",
> but presumably that is something we never expect to happen, either).
Yep, it should not be triggered at all, but as I said, it is a nice defensive
programming measure to avoid segmentation faults in case of a bug.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git: regression with mergetool and answering "n" (backport fix / add tests)
Hi,
I am a bit surprised that this bug still exists in "maint" / v2.2.2.
Cherry-picking/merging 0ddedd4 fixes it.
Regards,
Daniel.
On 26.12.2014 02:12, Daniel Hahler wrote:
> Hi David,
>
> sorry for the confusion - the patch / fix I've mentioned was meant to be
> applied on the commit that caused the regression and not current master.
>
>
> Cheers,
> Daniel.
>
> On 26.12.2014 02:00, David Aguilar wrote:
>> On Tue, Dec 23, 2014 at 08:08:34PM +0100, Daniel Hahler wrote:
>>> Hi,
>>>
>>> this is in reply to the commits from David:
>>>
>>> commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8
>>> Refs: v2.2.0-60-g0ddedd4
>>> Merge: e886efd 1e86d5b
>>> Author: Junio C Hamano
>>> AuthorDate: Fri Dec 12 14:31:39 2014 -0800
>>> Commit: Junio C Hamano
>>> CommitDate: Fri Dec 12 14:31:39 2014 -0800
>>>
>>> Merge branch 'da/difftool-mergetool-simplify-reporting-status'
>>>
>>> Code simplification.
>>>
>>> * da/difftool-mergetool-simplify-reporting-status:
>>> mergetools: stop setting $status in merge_cmd()
>>> mergetool: simplify conditionals
>>> difftool--helper: add explicit exit statement
>>> mergetool--lib: remove use of $status global
>>> mergetool--lib: remove no-op assignment to $status from
>>> setup_user_tool
>>>
>>> I've ran into a problem, where "git mergetool" (using vimdiff) would add
>>> the changes to the index, although you'd answered "n" after not
>>> changing/saving
>>> the merged file.
>>
>> Thanks for the heads-up.
>>
>> Do you perhaps have mergetool.vimdiff.trustExitCode defined, or
>> a similar setting?
>>
>> If you saw the prompt then it should have aborted right after
>> you answered "n".
>>
>> The very last thing merge_cmd() for vimdiff does is call
>> check_unchanged(). We'll come back to check_unchanged() later.
>>
>> I tried to reproduce this issue. Here's a transcript:
>>
>>
>> $ git status -s
>> UU file.txt
>>
>> $ git mergetool -t vimdiff file.txt
>> Merging:
>> file.txt
>>
>> Normal merge conflict for 'file.txt':
>> {local}: modified file
>> {remote}: modified file
>> 4 files to edit
>> Enter :qall inside vim
>> file.txt seems unchanged.
>> Was the merge successful? [y/n] n
>> merge of file.txt failed
>> Continue merging other unresolved paths (y/n) ? n
>>
>> $ git status -s
>> UU file.txt
>>
>>
>> That seemed to work fine. Any clues?
>> More notes below...
>>
>>> This regression has been introduced in:
>>>
>>> commit 99474b6340dbcbe58f6c256fdee231cbadb060f4
>>> Author: David Aguilar
>>> Date: Fri Nov 14 13:33:55 2014 -0800
>>>
>>> difftool: honor --trust-exit-code for builtin tools
>>>
>>> run_merge_tool() was not setting $status, which prevented the
>>> exit code for builtin tools from being forwarded to the caller.
>>>
>>> Capture the exit status and add a test to guarantee the behavior.
>>>
>>> Reported-by: Adria Farres <14farr...@gmail.com>
>>> Signed-off-by: David Aguilar
>>> Signed-off-by: Junio C Hamano
>>>
>>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>>> index c45a020..cce4f8c 100644
>>> --- a/git-mergetool--lib.sh
>>> +++ b/git-mergetool--lib.sh
>>> @@ -221,6 +221,7 @@ run_merge_tool () {
>>> else
>>> run_diff_cmd "$1"
>>> fi
>>> + status=$?
>>> return $status
>>> }
>>>
>>>
>>> My fix has been the following, but I agree that the changes from David
>>> are much better in general.
>>>
>>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>>> index cce4f8c..fa9acb1 100644
>>> --- a/git-mergetool--lib.sh
>>> +++ b/git-mergetool--lib.sh
>>> @@ -105,6 +105,7 @@ check_unchanged () {
>>> esac
>>> done
>>> fi
>>> + return $status
>>> }
>>
>> I don't think this fix does anything.
>> Here is all of check_unchanged() for context:
>>
>> check_unchanged () {
>> if test "$MERGED" -nt "$BACKUP"
>> then
>> return 0
>> else
>> while true
>> do
>> echo "$MERGED seems unchanged."
>> printf "Was the merge successful? [y/n] "
>> read answer || return 1
>> case "$answer" in
>> y*|Y*) return 0 ;;
>> n*|N*) return 1 ;;
>> esac
>> done
>> fi
>> }
>>
>> The addition of "return $status" after the "fi" in the above fix
>> won't do anything because that code is unreachable.
>> We either return 0 or 1.
>>
>>> I haven't verified if it really fixes the regression, but if it does it
>>> should get backported into the branches where the regression is present.
>>
>> Also, the $status variable doesn't even exist anymore, so the
>> fix is suspect.
>>
>> Wh
git push --repo option not working as described in git manual.
I am using git 2.2.2 and want to report an issue with git push --repo option. git 2.2.2 manual says that git push --repo=public will push to public only if the current branch does not track a remote branch. See http://git-scm.com/docs/git-push However, I see that git pushes even when the current branch is tracking a remote branch. Here is the test case (push default setting is simple, git version 2.2.2, Mac OS X 10.10.1): 1. I have a local branch "master". 2. "master" tracks remote branch "blah/master". Here "blah" is the remote repository. 3. While I am on my local master branch, I run git push --repo=silver 4. git pushes the local master branch to silver repository. 5. But per git manual, it shouldn't push to silver, as the local branch is tracking "blah/master". Here is another test case (push default setting is simple, git version 2.2.2, Mac OS X 10.10.1): 1. I have a local branch "whoa". 2. "whoa" tracks remote branch "origin/whoa". Here "origin" is the remote repository. 3. While I am on my local whoa branch, I run git push --repo=blah 4. git pushes the local whoa branch to blah repository. 5. But per git manual, it shouldn't push to blah, as the local branch is tracking "origin/whoa". Appreciate your help. Prem -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GUILT 0/5] doc: less guilt-foo invocations, minor Makefile fixes
guilt no longer supports running commands on the "guilt-add" form. You need to use "guilt add" instead. This patch series updates most of the documentation to use the supported "guilt add" form. There is one known instance where I did not change the style: in the NAME section in Documentation/guilt-*.txt. The reason is that if I change it there, xmlto will create the man pages as e.g. guilt_add.1 instead of guilt-add.1, and I don't know how to fix that. Also, the git man pages (as of Git 2.1.0) still have "git-add" under the NAME heading of git-add(1), so it might be wise to follow suite. While working on this, I also found two minor issues with Documentation/Makefile. /ceder Per Cederqvist (5): Fix generation of Documentation/usage-%.txt. doc: guilt.xml depends on cmds.txt. doc: don't use guilt-foo invocations in examples. doc: don't use guilt-foo invocations in usage messages. doc: git doesn't use git-foo invocations. Documentation/.gitignore| 3 +++ Documentation/Makefile | 6 -- Documentation/guilt-add.txt | 4 ++-- Documentation/guilt-delete.txt | 2 +- Documentation/guilt-diff.txt| 2 +- Documentation/guilt-help.txt| 4 ++-- Documentation/guilt-new.txt | 6 +++--- Documentation/guilt-refresh.txt | 2 +- Documentation/guilt-repair.txt | 2 +- Documentation/guilt-rm.txt | 2 +- Documentation/guilt-select.txt | 4 ++-- Documentation/usage.sh | 8 +++- 12 files changed, 24 insertions(+), 21 deletions(-) -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GUILT 2/5] doc: guilt.xml depends on cmds.txt.
Specify an explicit dependency, to stop make from trying to generate guilt.xml if cmds.txt could not be created. The asciidoc will fail and produce an error message that might hide the original error message. The added dependency causes make to not remove the guilt.xml file. Add *.xml to .gitignore. Signed-off-by: Per Cederqvist --- Documentation/.gitignore | 3 +++ Documentation/Makefile | 2 ++ 2 files changed, 5 insertions(+) diff --git a/Documentation/.gitignore b/Documentation/.gitignore index c4f0588..9b8d4da 100644 --- a/Documentation/.gitignore +++ b/Documentation/.gitignore @@ -11,3 +11,6 @@ version.txt # Generated file dependency list doc.dep + +# Intermediate generated files +*.xml diff --git a/Documentation/Makefile b/Documentation/Makefile index ec3c9e8..2574125 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -60,6 +60,8 @@ cmds.txt: cmd-list.sh $(MAN1_TXT) guilt.7 guilt.html: guilt.txt footer.txt version.txt +guilt.xml: cmds.txt + clean: rm -f *.xml *.html *.1 *.7 doc.dep rm -f cmds.txt -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GUILT 1/5] Fix generation of Documentation/usage-%.txt.
The old rule worked, most of the time, but had several issues: - It depended on the corresponding guilt-*.txt file, but the usage.sh script actually reads ../guilt-foo. - Actually, each usage-%.txt depended on all guilt-*.txt files, so make had to do more work than necessary if a single file was altered. - The construct broke parallel make, which would spawn several usage.sh at once. This leads to unnecessary work, and could potentially result in broken usage files if the "echo some_string > some_file" construct used by usage.sh isn't atomic. Fixed by letting the usage.sh script update a single file, and writing a proper implicit make rule. This makes parallel make work a lot better. There is a small downside, though, as usage.sh will now be run once for each command (if everything is regenerated). I think it is worth to pay that price to get the correctness. This command is still very fast compared to the docbook processing. Signed-off-by: Per Cederqvist --- Documentation/Makefile | 4 ++-- Documentation/usage.sh | 8 +++- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/Documentation/Makefile b/Documentation/Makefile index b6c3285..ec3c9e8 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -66,8 +66,8 @@ clean: rm -f usage-*.txt rm -f version.txt -usage-%.txt: $(MAN1_TXT) usage.sh - sh ./usage.sh +usage-guilt-%.txt: ../guilt-% usage.sh + sh ./usage.sh $< %.html : %.txt footer.txt version.txt $(ASCIIDOC) -b xhtml11 -d manpage -f asciidoc.conf $(ASCIIDOC_EXTRA) $< diff --git a/Documentation/usage.sh b/Documentation/usage.sh index 20fdca4..629f546 100644 --- a/Documentation/usage.sh +++ b/Documentation/usage.sh @@ -1,7 +1,5 @@ #!/bin/sh -for i in `ls ../guilt-*`; do - name=$(basename $i) - u=$(grep USAGE $i | sed 's/USAGE="//' | sed 's/"$//') - echo "'$name' $u" > usage-$name.txt -done +name=$(basename $1) +u=$(grep USAGE $1 | sed 's/USAGE="//' | sed 's/"$//') +echo "'$name' $u" > usage-$name.txt -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GUILT 3/5] doc: don't use guilt-foo invocations in examples.
Note: there is one place where I replace guilt-repair with "guilt repair" instead of "+guilt repair+". At least the version of docbook I'm using mishandles the "+" signs in that particular spot (even though it works properly for "+guilt select+" in another file. I know too little docbook to be able to find the cause. Signed-off-by: Per Cederqvist --- Documentation/guilt-add.txt| 2 +- Documentation/guilt-delete.txt | 2 +- Documentation/guilt-diff.txt | 2 +- Documentation/guilt-help.txt | 4 ++-- Documentation/guilt-new.txt| 6 +++--- Documentation/guilt-repair.txt | 2 +- Documentation/guilt-select.txt | 4 ++-- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Documentation/guilt-add.txt b/Documentation/guilt-add.txt index 6d2785a..a276f09 100644 --- a/Documentation/guilt-add.txt +++ b/Documentation/guilt-add.txt @@ -24,7 +24,7 @@ EXAMPLES Create and add a new file example.c $ touch example.c - $ guilt-add example.c + $ guilt add example.c Author -- diff --git a/Documentation/guilt-delete.txt b/Documentation/guilt-delete.txt index ef57dc6..4e8c28c 100644 --- a/Documentation/guilt-delete.txt +++ b/Documentation/guilt-delete.txt @@ -25,7 +25,7 @@ EXAMPLES Delete a patch called 'foobar': - $ guilt-delete foobar + $ guilt delete foobar Author -- diff --git a/Documentation/guilt-diff.txt b/Documentation/guilt-diff.txt index 986ceca..0ee062c 100644 --- a/Documentation/guilt-diff.txt +++ b/Documentation/guilt-diff.txt @@ -18,7 +18,7 @@ OPTIONS --- -z:: Output a interdiff against the top-most applied patch. This should - produce the same diff as "+guilt-new -f foo+". + produce the same diff as "+guilt new -f foo+". ...:: Restrict diff output to a given set of files. diff --git a/Documentation/guilt-help.txt b/Documentation/guilt-help.txt index ed6a5cf..df0e0fb 100644 --- a/Documentation/guilt-help.txt +++ b/Documentation/guilt-help.txt @@ -18,11 +18,11 @@ EXAMPLES Open the guilt-status man page - $ guilt-help status + $ guilt help status Open the guilt man page - $ guilt-help + $ guilt help Author -- diff --git a/Documentation/guilt-new.txt b/Documentation/guilt-new.txt index a2c8a4c..698dcb7 100644 --- a/Documentation/guilt-new.txt +++ b/Documentation/guilt-new.txt @@ -42,16 +42,16 @@ EXAMPLES Create a new patch called 'foobar': - $ guilt-new foobar + $ guilt new foobar Create a patch called 'foo' and supply a patch description interactively: - $ guilt-new -e foo + $ guilt new -e foo Create a patch called 'bar' with a provided patch description and sign off on the patch: - $ guilt-new -s -m patch-fu bar + $ guilt new -s -m patch-fu bar Author -- diff --git a/Documentation/guilt-repair.txt b/Documentation/guilt-repair.txt index 4aa472b..4faf113 100644 --- a/Documentation/guilt-repair.txt +++ b/Documentation/guilt-repair.txt @@ -22,7 +22,7 @@ Perform various repository repairs. You must specify one mode of repair: WARNING: Running this command may result in commits and working directory changes being lost. You may want to create a new reference (e.g., branch, or reflog) to the original HEAD before using - guilt-repair. + "guilt repair". --status:: Upgrade the status file from old format to new. diff --git a/Documentation/guilt-select.txt b/Documentation/guilt-select.txt index f7fb5f7..dd5833e 100644 --- a/Documentation/guilt-select.txt +++ b/Documentation/guilt-select.txt @@ -19,10 +19,10 @@ the following way: * An unguarded patch is always applied. * A patch with a positive guard is applied *only* if the guard is -selected with guilt-select. +selected with "+guilt select+". * A patch with a negative guard is applied *unless* the guard is -selected with guilt-select. +selected with "+guilt select+". OPTIONS --- -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GUILT 4/5] doc: don't use guilt-foo invocations in usage messages.
Signed-off-by: Per Cederqvist --- Documentation/usage.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/usage.sh b/Documentation/usage.sh index 629f546..9cc49f7 --- a/Documentation/usage.sh +++ b/Documentation/usage.sh @@ -2,4 +2,4 @@ name=$(basename $1) u=$(grep USAGE $1 | sed 's/USAGE="//' | sed 's/"$//') -echo "'$name' $u" > usage-$name.txt +echo "'`echo $name|sed -e 's/^guilt-/guilt /'`' $u" > usage-$name.txt -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GUILT 5/5] doc: git doesn't use git-foo invocations.
Make them into reference to the man pages instead. Signed-off-by: Per Cederqvist --- Documentation/guilt-add.txt | 2 +- Documentation/guilt-refresh.txt | 2 +- Documentation/guilt-rm.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/guilt-add.txt b/Documentation/guilt-add.txt index a276f09..067b6ca 100644 --- a/Documentation/guilt-add.txt +++ b/Documentation/guilt-add.txt @@ -11,7 +11,7 @@ include::usage-guilt-add.txt[] DESCRIPTION --- -Adds the files specified to git using git-add making it available to guilt. +Adds the files specified to git using git-add(1) making it available to guilt. OPTIONS --- diff --git a/Documentation/guilt-refresh.txt b/Documentation/guilt-refresh.txt index 7757bdc..98076e3 100644 --- a/Documentation/guilt-refresh.txt +++ b/Documentation/guilt-refresh.txt @@ -23,7 +23,7 @@ OPTIONS Include a diffstat output in the patch file. Useful for cases where patches will be submitted with other tools. + -If the command line option is omitted, the corresponding git-config +If the command line option is omitted, the corresponding git-config(1) option "guilt.diffstat" will be queried. So this would enable diffstat output by default: diff --git a/Documentation/guilt-rm.txt b/Documentation/guilt-rm.txt index 71b49fe..cfe471e 100644 --- a/Documentation/guilt-rm.txt +++ b/Documentation/guilt-rm.txt @@ -11,7 +11,7 @@ include::usage-guilt-rm.txt[] DESCRIPTION --- -Removes the files specified from git using git-rm +Removes the files specified from git using git-rm(1). OPTIONS --- -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git compile warnings (under mac/clang)
On Fri, Jan 23, 2015 at 01:38:17PM +0100, Johannes Schindelin wrote:
> > Unless we are willing to drop the ">= 0" check completely. I think it is
> > valid to do so regardless of the compiler's representation decision due
> > to the numbering rules I mentioned above. It kind-of serves as a
> > cross-check that we haven't cast some random int into the enum, but I
> > think we would do better to find those callsites (since they are not
> > guaranteed to work, anyway; in addition to signedness, it might choose a
> > much smaller representation).
>
> Yeah, well, this check is really more of a safety net in case I messed
> up anything; I was saved so many times by my own defensive programming
> that I try to employ it as much as I can.
Yeah, I am all in favor of defensive programming. But I am not sure that
it is defending much here, as we silently fall back to an alternate
value for the severity. Would we notice, or would that produce subtly
wrong results? IOW, would this be better as:
assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);
or something?
> -- snip --
> diff --git a/fsck.c b/fsck.c
> index 15cb8bd..8f8c82f 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -107,7 +107,7 @@ static int fsck_msg_severity(enum fsck_msg_id msg_id,
> {
> int severity;
>
> - if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
> + if (options->msg_severity && ((unsigned int) msg_id) < FSCK_MSG_MAX)
> severity = options->msg_severity[msg_id];
> else {
> severity = msg_id_info[msg_id].severity;
> -- snap --
>
> What do you think? Michael, does this cause more Clang warnings, or would it
> resolve the issue?
Hmm, yeah, that does not seem unreasonable, and is more localized.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT 1/5] Fix generation of Documentation/usage-%.txt.
On Fri, Jan 23, 2015 at 02:24:55PM +0100, Per Cederqvist wrote:
> The old rule worked, most of the time, but had several issues:
>
> - It depended on the corresponding guilt-*.txt file, but the usage.sh
>script actually reads ../guilt-foo.
>
> - Actually, each usage-%.txt depended on all guilt-*.txt files, so
>make had to do more work than necessary if a single file was
>altered.
>
> - The construct broke parallel make, which would spawn several
>usage.sh at once. This leads to unnecessary work, and could
>potentially result in broken usage files if the "echo some_string >
>some_file" construct used by usage.sh isn't atomic.
>
> Fixed by letting the usage.sh script update a single file, and writing
> a proper implicit make rule. This makes parallel make work a lot
> better.
Nice!
> There is a small downside, though, as usage.sh will now be run once
> for each command (if everything is regenerated). I think it is worth
> to pay that price to get the correctness. This command is still very
> fast compared to the docbook processing.
Given how much simple usage.sh got, I'm thinking it might be worth it to
just remove it, and just shove the rule into the makefile itself.
Ok, I tried to write it. I came up with the following. (Note: I have *not*
tested it.) It's not *that* ugly.
usage-guilt-%.txt: ../guilt-% usage.sh
echo "'$(basename $<)' `sed -n -e '/^USAGE=/{s/USAGE="//; s/"$//; p;
q}' $<`" > $@
What do you think? Too opaque? Your change looks good.
Jeff.
> Signed-off-by: Per Cederqvist
> ---
> Documentation/Makefile | 4 ++--
> Documentation/usage.sh | 8 +++-
> 2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index b6c3285..ec3c9e8 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -66,8 +66,8 @@ clean:
> rm -f usage-*.txt
> rm -f version.txt
>
> -usage-%.txt: $(MAN1_TXT) usage.sh
> - sh ./usage.sh
> +usage-guilt-%.txt: ../guilt-% usage.sh
> + sh ./usage.sh $<
>
> %.html : %.txt footer.txt version.txt
> $(ASCIIDOC) -b xhtml11 -d manpage -f asciidoc.conf $(ASCIIDOC_EXTRA) $<
> diff --git a/Documentation/usage.sh b/Documentation/usage.sh
> index 20fdca4..629f546 100644
> --- a/Documentation/usage.sh
> +++ b/Documentation/usage.sh
> @@ -1,7 +1,5 @@
> #!/bin/sh
>
> -for i in `ls ../guilt-*`; do
> - name=$(basename $i)
> - u=$(grep USAGE $i | sed 's/USAGE="//' | sed 's/"$//')
> - echo "'$name' $u" > usage-$name.txt
> -done
> +name=$(basename $1)
> +u=$(grep USAGE $1 | sed 's/USAGE="//' | sed 's/"$//')
> +echo "'$name' $u" > usage-$name.txt
> --
> 2.1.0
>
--
The reasonable man adapts himself to the world; the unreasonable one
persists in trying to adapt the world to himself. Therefore all progress
depends on the unreasonable man.
- George Bernard Shaw
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[GUILT 0/2] Teach "guilt graph" to ignore some files.
If you use a ChangeLog, all output from "guilt graph" will be a boring line of commits. By using "guilt graph -x ChangeLog" things will look more interesting. Also: simplify getfiles. (This work is also available on the guilt-graph-ignore-2015-v1 branch of the git://repo.or.cz/guilt/ceder.git repository. (That branch is based on the doc-dash-2015-v1 branch that contains my documentation fixes, so if you just want these two commits you will have to cherry-pick.)) /ceder Per Cederqvist (2): guilt graph: Simplify getfiles. Teach "guilt graph" the "-x exclude-pattern" option. Documentation/guilt-graph.txt | 5 + guilt-graph | 26 +++--- regression/t-033.out | 12 regression/t-033.sh | 3 +++ 4 files changed, 39 insertions(+), 7 deletions(-) -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GUILT 2/2] Teach "guilt graph" the "-x exclude-pattern" option.
Some projects keep a ChangeLog which every commit modifies. This
makes the graph a very uninteresting single line of commits. It is
sometimes useful to see how the graph would look if we ignore the
ChangeLog file.
The new -x option is useful in situations like this. It can be
repeated several times to ignore many files. Each argument is saved
to a temporary file and "grep -v -f $TEMPORARY" is used to filter out
the file names you want to ignore.
Also added a minimal test case and documentation.
Signed-off-by: Per Cederqvist
---
Documentation/guilt-graph.txt | 5 +
guilt-graph | 24 ++--
regression/t-033.out | 12
regression/t-033.sh | 3 +++
4 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/Documentation/guilt-graph.txt b/Documentation/guilt-graph.txt
index f43206e..eeed321 100644
--- a/Documentation/guilt-graph.txt
+++ b/Documentation/guilt-graph.txt
@@ -16,6 +16,11 @@ patches.
OPTIONS
---
+-x ::
+ Ignore files that matches the given grep pattern. Can be
+ repeated to ignore several files. This can be useful to ignore
+ for instance ChangeLog files that every commit modifies.
+
::
Instead of starting with the topmost applied patch, start with
.
diff --git a/guilt-graph b/guilt-graph
index d90c2f1..4d5fe46 100755
--- a/guilt-graph
+++ b/guilt-graph
@@ -3,7 +3,7 @@
# Copyright (c) Josef "Jeff" Sipek, 2007-2013
#
-USAGE="[]"
+USAGE="[-x exclude-pattern]... []"
if [ -z "$GUILT_VERSION" ]; then
echo "Invoking `basename "$0"` directly is no longer supported." >&2
exit 1
@@ -11,6 +11,22 @@ fi
_main() {
+cache="$GUILT_DIR/$branch/.graphcache.$$"
+xclude="$GUILT_DIR/$branch/.graphexclude.$$"
+trap "rm -rf \"$cache\" \"$xclude\"" 0
+mkdir "$cache"
+>"$xclude"
+
+while [ $# -gt 0 ]; do
+if [ "$1" = "-x" ] && [ $# -ge 2 ]; then
+ echo "$2" >> "$xclude"
+ shift
+ shift
+else
+ break
+fi
+done
+
if [ $# -gt 1 ]; then
usage
fi
@@ -39,10 +55,6 @@ getfiles()
git diff-tree -r "$1^" "$1" | cut -f2
}
-cache="$GUILT_DIR/$branch/.graphcache.$$"
-mkdir "$cache"
-trap "rm -rf \"$cache\"" 0
-
disp "digraph G {"
current="$top"
@@ -66,7 +78,7 @@ while [ "$current" != "$base" ]; do
rm -f "$cache/dep"
touch "$cache/dep"
- getfiles $current | while read f; do
+ getfiles $current | grep -v -f "$xclude" | while read f; do
# hash the filename
fh=`echo "$f" | sha1 | cut -d' ' -f1`
if [ -e "$cache/$fh" ]; then
diff --git a/regression/t-033.out b/regression/t-033.out
index c120d4f..1ed371f 100644
--- a/regression/t-033.out
+++ b/regression/t-033.out
@@ -88,3 +88,15 @@ digraph G {
"ff2775f8d1dc753f635830adcc3a067e0b681e2d" [label="a.patch"]
"891bc14b5603474c9743fd04f3da888644413dc5" ->
"ff2775f8d1dc753f635830adcc3a067e0b681e2d"; // ?
}
+%% The same graph, but excluding deps introduced by file.txt.
+% guilt graph -x file.txt
+digraph G {
+# checking rev bc7df666a646739eaf559af23cab72f2bfd01f0e
+ "bc7df666a646739eaf559af23cab72f2bfd01f0e"
[label="a-\"better&quicker'-patch.patch"]
+# checking rev 891bc14b5603474c9743fd04f3da888644413dc5
+ "891bc14b5603474c9743fd04f3da888644413dc5" [label="c.patch"]
+# checking rev c7014443c33d2b0237293687ceb9cbd38313df65
+ "c7014443c33d2b0237293687ceb9cbd38313df65" [label="b.patch"]
+# checking rev ff2775f8d1dc753f635830adcc3a067e0b681e2d
+ "ff2775f8d1dc753f635830adcc3a067e0b681e2d" [label="a.patch"]
+}
diff --git a/regression/t-033.sh b/regression/t-033.sh
index 9fe1827..ae22914 100755
--- a/regression/t-033.sh
+++ b/regression/t-033.sh
@@ -59,3 +59,6 @@ cmd git add file.txt
cmd guilt refresh
fixup_time_info "a-\"better&quicker'-patch.patch"
cmd guilt graph
+
+echo "%% The same graph, but excluding deps introduced by file.txt."
+cmd guilt graph -x file.txt
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[GUILT 1/2] guilt graph: Simplify getfiles.
git diff-tree by default emits TAB-separated fields. cut by defaults
processes TAB-separated fields. Simplify getfiles() by using TAB as
the separator.
Signed-off-by: Per Cederqvist
---
guilt-graph | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/guilt-graph b/guilt-graph
index 0857e0d..d90c2f1 100755
--- a/guilt-graph
+++ b/guilt-graph
@@ -36,7 +36,7 @@ fi
getfiles()
{
- git diff-tree -r "$1^" "$1" | tr '\t' ' ' | cut -d' ' -f6
+ git diff-tree -r "$1^" "$1" | cut -f2
}
cache="$GUILT_DIR/$branch/.graphcache.$$"
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT 2/5] doc: guilt.xml depends on cmds.txt.
Signed-off-by: Josef 'Jeff' Sipek On Fri, Jan 23, 2015 at 02:24:56PM +0100, Per Cederqvist wrote: > Specify an explicit dependency, to stop make from trying to generate > guilt.xml if cmds.txt could not be created. The asciidoc will fail > and produce an error message that might hide the original error > message. > > The added dependency causes make to not remove the guilt.xml file. > Add *.xml to .gitignore. > > Signed-off-by: Per Cederqvist > --- > Documentation/.gitignore | 3 +++ > Documentation/Makefile | 2 ++ > 2 files changed, 5 insertions(+) > > diff --git a/Documentation/.gitignore b/Documentation/.gitignore > index c4f0588..9b8d4da 100644 > --- a/Documentation/.gitignore > +++ b/Documentation/.gitignore > @@ -11,3 +11,6 @@ version.txt > > # Generated file dependency list > doc.dep > + > +# Intermediate generated files > +*.xml > diff --git a/Documentation/Makefile b/Documentation/Makefile > index ec3c9e8..2574125 100644 > --- a/Documentation/Makefile > +++ b/Documentation/Makefile > @@ -60,6 +60,8 @@ cmds.txt: cmd-list.sh $(MAN1_TXT) > > guilt.7 guilt.html: guilt.txt footer.txt version.txt > > +guilt.xml: cmds.txt > + > clean: > rm -f *.xml *.html *.1 *.7 doc.dep > rm -f cmds.txt > -- > 2.1.0 > -- The obvious mathematical breakthrough would be development of an easy way to factor large prime numbers. - Bill Gates, The Road Ahead, pg. 265 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT 3/5] doc: don't use guilt-foo invocations in examples.
On Fri, Jan 23, 2015 at 02:24:57PM +0100, Per Cederqvist wrote: > Note: there is one place where I replace guilt-repair with "guilt > repair" instead of "+guilt repair+". At least the version of docbook > I'm using mishandles the "+" signs in that particular spot (even > though it works properly for "+guilt select+" in another file. I know > too little docbook to be able to find the cause. Yeah, a bit of a mystery to me too. Regardless, Signed-off-by: Josef 'Jeff' Sipek > Signed-off-by: Per Cederqvist > --- > Documentation/guilt-add.txt| 2 +- > Documentation/guilt-delete.txt | 2 +- > Documentation/guilt-diff.txt | 2 +- > Documentation/guilt-help.txt | 4 ++-- > Documentation/guilt-new.txt| 6 +++--- > Documentation/guilt-repair.txt | 2 +- > Documentation/guilt-select.txt | 4 ++-- > 7 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/Documentation/guilt-add.txt b/Documentation/guilt-add.txt > index 6d2785a..a276f09 100644 > --- a/Documentation/guilt-add.txt > +++ b/Documentation/guilt-add.txt > @@ -24,7 +24,7 @@ EXAMPLES > Create and add a new file example.c > > $ touch example.c > - $ guilt-add example.c > + $ guilt add example.c > > Author > -- > diff --git a/Documentation/guilt-delete.txt b/Documentation/guilt-delete.txt > index ef57dc6..4e8c28c 100644 > --- a/Documentation/guilt-delete.txt > +++ b/Documentation/guilt-delete.txt > @@ -25,7 +25,7 @@ EXAMPLES > > Delete a patch called 'foobar': > > - $ guilt-delete foobar > + $ guilt delete foobar > > Author > -- > diff --git a/Documentation/guilt-diff.txt b/Documentation/guilt-diff.txt > index 986ceca..0ee062c 100644 > --- a/Documentation/guilt-diff.txt > +++ b/Documentation/guilt-diff.txt > @@ -18,7 +18,7 @@ OPTIONS > --- > -z:: > Output a interdiff against the top-most applied patch. This should > - produce the same diff as "+guilt-new -f foo+". > + produce the same diff as "+guilt new -f foo+". > > ...:: > Restrict diff output to a given set of files. > diff --git a/Documentation/guilt-help.txt b/Documentation/guilt-help.txt > index ed6a5cf..df0e0fb 100644 > --- a/Documentation/guilt-help.txt > +++ b/Documentation/guilt-help.txt > @@ -18,11 +18,11 @@ EXAMPLES > > Open the guilt-status man page > > - $ guilt-help status > + $ guilt help status > > Open the guilt man page > > - $ guilt-help > + $ guilt help > > Author > -- > diff --git a/Documentation/guilt-new.txt b/Documentation/guilt-new.txt > index a2c8a4c..698dcb7 100644 > --- a/Documentation/guilt-new.txt > +++ b/Documentation/guilt-new.txt > @@ -42,16 +42,16 @@ EXAMPLES > > Create a new patch called 'foobar': > > - $ guilt-new foobar > + $ guilt new foobar > > Create a patch called 'foo' and supply a patch description interactively: > > - $ guilt-new -e foo > + $ guilt new -e foo > > Create a patch called 'bar' with a provided patch description and sign off > on the patch: > > - $ guilt-new -s -m patch-fu bar > + $ guilt new -s -m patch-fu bar > > Author > -- > diff --git a/Documentation/guilt-repair.txt b/Documentation/guilt-repair.txt > index 4aa472b..4faf113 100644 > --- a/Documentation/guilt-repair.txt > +++ b/Documentation/guilt-repair.txt > @@ -22,7 +22,7 @@ Perform various repository repairs. You must specify one > mode of repair: > WARNING: Running this command may result in commits and working > directory changes being lost. You may want to create a new reference > (e.g., branch, or reflog) to the original HEAD before using > - guilt-repair. > + "guilt repair". > > --status:: > Upgrade the status file from old format to new. > diff --git a/Documentation/guilt-select.txt b/Documentation/guilt-select.txt > index f7fb5f7..dd5833e 100644 > --- a/Documentation/guilt-select.txt > +++ b/Documentation/guilt-select.txt > @@ -19,10 +19,10 @@ the following way: > * An unguarded patch is always applied. > > * A patch with a positive guard is applied *only* if the guard is > -selected with guilt-select. > +selected with "+guilt select+". > > * A patch with a negative guard is applied *unless* the guard is > -selected with guilt-select. > +selected with "+guilt select+". > > OPTIONS > --- > -- > 2.1.0 > -- mainframe, n.: An obsolete device still used by thousands of obsolete companies serving billions of obsolete customers and making huge obsolete profits for their obsolete shareholders. And this year's run twice as fast as last year's. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT 4/5] doc: don't use guilt-foo invocations in usage messages.
Ah, I see you changed usage.sh here. I guess that kinda invalidates my comment for patch 1/5. On Fri, Jan 23, 2015 at 02:24:58PM +0100, Per Cederqvist wrote: > Signed-off-by: Per Cederqvist > --- > Documentation/usage.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/usage.sh b/Documentation/usage.sh > index 629f546..9cc49f7 > --- a/Documentation/usage.sh > +++ b/Documentation/usage.sh > @@ -2,4 +2,4 @@ > > name=$(basename $1) > u=$(grep USAGE $1 | sed 's/USAGE="//' | sed 's/"$//') > -echo "'$name' $u" > usage-$name.txt > +echo "'`echo $name|sed -e 's/^guilt-/guilt /'`' $u" > usage-$name.txt Tiny nitpick: spaces around the |, otherwise looks good. Signed-off-by: Josef 'Jeff' Sipek > -- > 2.1.0 > -- Si hoc legere scis nimium eruditionis habes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT 5/5] doc: git doesn't use git-foo invocations.
Signed-off-by: Josef 'Jeff' Sipek On Fri, Jan 23, 2015 at 02:24:59PM +0100, Per Cederqvist wrote: > Make them into reference to the man pages instead. > > Signed-off-by: Per Cederqvist > --- > Documentation/guilt-add.txt | 2 +- > Documentation/guilt-refresh.txt | 2 +- > Documentation/guilt-rm.txt | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/guilt-add.txt b/Documentation/guilt-add.txt > index a276f09..067b6ca 100644 > --- a/Documentation/guilt-add.txt > +++ b/Documentation/guilt-add.txt > @@ -11,7 +11,7 @@ include::usage-guilt-add.txt[] > > DESCRIPTION > --- > -Adds the files specified to git using git-add making it available to guilt. > +Adds the files specified to git using git-add(1) making it available to > guilt. > > OPTIONS > --- > diff --git a/Documentation/guilt-refresh.txt b/Documentation/guilt-refresh.txt > index 7757bdc..98076e3 100644 > --- a/Documentation/guilt-refresh.txt > +++ b/Documentation/guilt-refresh.txt > @@ -23,7 +23,7 @@ OPTIONS > Include a diffstat output in the patch file. Useful for cases where > patches will be submitted with other tools. > + > -If the command line option is omitted, the corresponding git-config > +If the command line option is omitted, the corresponding git-config(1) > option "guilt.diffstat" will be queried. So this would enable diffstat > output by default: > > diff --git a/Documentation/guilt-rm.txt b/Documentation/guilt-rm.txt > index 71b49fe..cfe471e 100644 > --- a/Documentation/guilt-rm.txt > +++ b/Documentation/guilt-rm.txt > @@ -11,7 +11,7 @@ include::usage-guilt-rm.txt[] > > DESCRIPTION > --- > -Removes the files specified from git using git-rm > +Removes the files specified from git using git-rm(1). > > OPTIONS > --- > -- > 2.1.0 > -- The obvious mathematical breakthrough would be development of an easy way to factor large prime numbers. - Bill Gates, The Road Ahead, pg. 265 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT 1/5] Fix generation of Documentation/usage-%.txt.
On Fri, Jan 23, 2015 at 3:21 PM, Jeff Sipek wrote:
> On Fri, Jan 23, 2015 at 02:24:55PM +0100, Per Cederqvist wrote:
>> The old rule worked, most of the time, but had several issues:
>>
>> - It depended on the corresponding guilt-*.txt file, but the usage.sh
>>script actually reads ../guilt-foo.
>>
>> - Actually, each usage-%.txt depended on all guilt-*.txt files, so
>>make had to do more work than necessary if a single file was
>>altered.
>>
>> - The construct broke parallel make, which would spawn several
>>usage.sh at once. This leads to unnecessary work, and could
>>potentially result in broken usage files if the "echo some_string >
>>some_file" construct used by usage.sh isn't atomic.
>>
>> Fixed by letting the usage.sh script update a single file, and writing
>> a proper implicit make rule. This makes parallel make work a lot
>> better.
>
> Nice!
>
>> There is a small downside, though, as usage.sh will now be run once
>> for each command (if everything is regenerated). I think it is worth
>> to pay that price to get the correctness. This command is still very
>> fast compared to the docbook processing.
>
> Given how much simple usage.sh got, I'm thinking it might be worth it to
> just remove it, and just shove the rule into the makefile itself.
>
> Ok, I tried to write it. I came up with the following. (Note: I have *not*
> tested it.) It's not *that* ugly.
>
> usage-guilt-%.txt: ../guilt-% usage.sh
> echo "'$(basename $<)' `sed -n -e '/^USAGE=/{s/USAGE="//; s/"$//; p;
> q}' $<`" > $@
>
> What do you think? Too opaque? Your change looks good.
Too opaque, and not tested enough. It doesn't work, since make will
handle all $. You need to write $$ instead of $ in at least one of the
places. I would stick with usage.sh, as getting the quoting right when
you have make, shell, subshells, and sed all at the same time is just
too painful.
But it is of course up to you. You are the maintainer. :-)
/ceder
> Jeff.
>
>> Signed-off-by: Per Cederqvist
>> ---
>> Documentation/Makefile | 4 ++--
>> Documentation/usage.sh | 8 +++-
>> 2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/Makefile b/Documentation/Makefile
>> index b6c3285..ec3c9e8 100644
>> --- a/Documentation/Makefile
>> +++ b/Documentation/Makefile
>> @@ -66,8 +66,8 @@ clean:
>> rm -f usage-*.txt
>> rm -f version.txt
>>
>> -usage-%.txt: $(MAN1_TXT) usage.sh
>> - sh ./usage.sh
>> +usage-guilt-%.txt: ../guilt-% usage.sh
>> + sh ./usage.sh $<
>>
>> %.html : %.txt footer.txt version.txt
>> $(ASCIIDOC) -b xhtml11 -d manpage -f asciidoc.conf $(ASCIIDOC_EXTRA) $<
>> diff --git a/Documentation/usage.sh b/Documentation/usage.sh
>> index 20fdca4..629f546 100644
>> --- a/Documentation/usage.sh
>> +++ b/Documentation/usage.sh
>> @@ -1,7 +1,5 @@
>> #!/bin/sh
>>
>> -for i in `ls ../guilt-*`; do
>> - name=$(basename $i)
>> - u=$(grep USAGE $i | sed 's/USAGE="//' | sed 's/"$//')
>> - echo "'$name' $u" > usage-$name.txt
>> -done
>> +name=$(basename $1)
>> +u=$(grep USAGE $1 | sed 's/USAGE="//' | sed 's/"$//')
>> +echo "'$name' $u" > usage-$name.txt
>> --
>> 2.1.0
>>
>
> --
> The reasonable man adapts himself to the world; the unreasonable one
> persists in trying to adapt the world to himself. Therefore all progress
> depends on the unreasonable man.
> - George Bernard Shaw
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT 1/5] Fix generation of Documentation/usage-%.txt.
On Fri, Jan 23, 2015 at 03:33:03PM +0100, Per Cederqvist wrote:
> On Fri, Jan 23, 2015 at 3:21 PM, Jeff Sipek wrote:
> > On Fri, Jan 23, 2015 at 02:24:55PM +0100, Per Cederqvist wrote:
> >> The old rule worked, most of the time, but had several issues:
> >>
> >> - It depended on the corresponding guilt-*.txt file, but the usage.sh
> >>script actually reads ../guilt-foo.
> >>
> >> - Actually, each usage-%.txt depended on all guilt-*.txt files, so
> >>make had to do more work than necessary if a single file was
> >>altered.
> >>
> >> - The construct broke parallel make, which would spawn several
> >>usage.sh at once. This leads to unnecessary work, and could
> >>potentially result in broken usage files if the "echo some_string >
> >>some_file" construct used by usage.sh isn't atomic.
> >>
> >> Fixed by letting the usage.sh script update a single file, and writing
> >> a proper implicit make rule. This makes parallel make work a lot
> >> better.
> >
> > Nice!
> >
> >> There is a small downside, though, as usage.sh will now be run once
> >> for each command (if everything is regenerated). I think it is worth
> >> to pay that price to get the correctness. This command is still very
> >> fast compared to the docbook processing.
> >
> > Given how much simple usage.sh got, I'm thinking it might be worth it to
> > just remove it, and just shove the rule into the makefile itself.
> >
> > Ok, I tried to write it. I came up with the following. (Note: I have *not*
> > tested it.) It's not *that* ugly.
> >
> > usage-guilt-%.txt: ../guilt-% usage.sh
> > echo "'$(basename $<)' `sed -n -e '/^USAGE=/{s/USAGE="//; s/"$//;
> > p; q}' $<`" > $@
> >
> > What do you think? Too opaque? Your change looks good.
>
> Too opaque,
Between that and the other patch in the series that modifies usage.sh, your
patch is good as is.
Signed-off-by: Josef 'Jeff' Sipek
> and not tested enough. It doesn't work, since make will
> handle all $. You need to write $$ instead of $ in at least one of the
> places. I would stick with usage.sh, as getting the quoting right when
> you have make, shell, subshells, and sed all at the same time is just
> too painful.
And this is comming from the person that rewrote cmd/shouldfail in a way
that the average shell user will go "whaaa??" :P (To be fair, I don't know
of a simpler way to make cmd/shouldfail.)
> But it is of course up to you. You are the maintainer. :-)
Heh.
Jeff.
--
Real Programmers consider "what you see is what you get" to be just as bad a
concept in Text Editors as it is in women. No, the Real Programmer wants a
"you asked for it, you got it" text editor -- complicated, cryptic,
powerful, unforgiving, dangerous.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT 1/2] guilt graph: Simplify getfiles.
Neat.
Signed-off-by: Josef 'Jeff' Sipek
On Fri, Jan 23, 2015 at 03:21:06PM +0100, Per Cederqvist wrote:
> git diff-tree by default emits TAB-separated fields. cut by defaults
> processes TAB-separated fields. Simplify getfiles() by using TAB as
> the separator.
>
> Signed-off-by: Per Cederqvist
> ---
> guilt-graph | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/guilt-graph b/guilt-graph
> index 0857e0d..d90c2f1 100755
> --- a/guilt-graph
> +++ b/guilt-graph
> @@ -36,7 +36,7 @@ fi
>
> getfiles()
> {
> - git diff-tree -r "$1^" "$1" | tr '\t' ' ' | cut -d' ' -f6
> + git diff-tree -r "$1^" "$1" | cut -f2
> }
>
> cache="$GUILT_DIR/$branch/.graphcache.$$"
> --
> 2.1.0
>
--
C is quirky, flawed, and an enormous success.
- Dennis M. Ritchie.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT 2/2] Teach "guilt graph" the "-x exclude-pattern" option.
On Fri, Jan 23, 2015 at 03:21:07PM +0100, Per Cederqvist wrote:
> Some projects keep a ChangeLog which every commit modifies. This
> makes the graph a very uninteresting single line of commits. It is
> sometimes useful to see how the graph would look if we ignore the
> ChangeLog file.
>
> The new -x option is useful in situations like this. It can be
> repeated several times to ignore many files. Each argument is saved
> to a temporary file and "grep -v -f $TEMPORARY" is used to filter out
> the file names you want to ignore.
Cool idea.
> Also added a minimal test case and documentation.
>
> Signed-off-by: Per Cederqvist
> ---
> Documentation/guilt-graph.txt | 5 +
> guilt-graph | 24 ++--
> regression/t-033.out | 12
> regression/t-033.sh | 3 +++
> 4 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/guilt-graph.txt b/Documentation/guilt-graph.txt
> index f43206e..eeed321 100644
> --- a/Documentation/guilt-graph.txt
> +++ b/Documentation/guilt-graph.txt
> @@ -16,6 +16,11 @@ patches.
>
> OPTIONS
> ---
> +-x ::
> + Ignore files that matches the given grep pattern. Can be
> + repeated to ignore several files. This can be useful to ignore
> + for instance ChangeLog files that every commit modifies.
> +
> ::
> Instead of starting with the topmost applied patch, start with
> .
> diff --git a/guilt-graph b/guilt-graph
> index d90c2f1..4d5fe46 100755
> --- a/guilt-graph
> +++ b/guilt-graph
> @@ -3,7 +3,7 @@
> # Copyright (c) Josef "Jeff" Sipek, 2007-2013
> #
>
> -USAGE="[]"
> +USAGE="[-x exclude-pattern]... []"
> if [ -z "$GUILT_VERSION" ]; then
> echo "Invoking `basename "$0"` directly is no longer supported." >&2
> exit 1
> @@ -11,6 +11,22 @@ fi
>
> _main() {
>
> +cache="$GUILT_DIR/$branch/.graphcache.$$"
> +xclude="$GUILT_DIR/$branch/.graphexclude.$$"
> +trap "rm -rf \"$cache\" \"$xclude\"" 0
> +mkdir "$cache"
> +>"$xclude"
> +
> +while [ $# -gt 0 ]; do
> +if [ "$1" = "-x" ] && [ $# -ge 2 ]; then
> + echo "$2" >> "$xclude"
> + shift
> + shift
> +else
> + break
> +fi
Spaces used for indentation. Otherwise looks good.
Signed-off-by: Josef 'Jeff' Sipek
> +done
> +
> if [ $# -gt 1 ]; then
> usage
> fi
> @@ -39,10 +55,6 @@ getfiles()
> git diff-tree -r "$1^" "$1" | cut -f2
> }
>
> -cache="$GUILT_DIR/$branch/.graphcache.$$"
> -mkdir "$cache"
> -trap "rm -rf \"$cache\"" 0
> -
> disp "digraph G {"
>
> current="$top"
> @@ -66,7 +78,7 @@ while [ "$current" != "$base" ]; do
> rm -f "$cache/dep"
> touch "$cache/dep"
>
> - getfiles $current | while read f; do
> + getfiles $current | grep -v -f "$xclude" | while read f; do
> # hash the filename
> fh=`echo "$f" | sha1 | cut -d' ' -f1`
> if [ -e "$cache/$fh" ]; then
> diff --git a/regression/t-033.out b/regression/t-033.out
> index c120d4f..1ed371f 100644
> --- a/regression/t-033.out
> +++ b/regression/t-033.out
> @@ -88,3 +88,15 @@ digraph G {
> "ff2775f8d1dc753f635830adcc3a067e0b681e2d" [label="a.patch"]
> "891bc14b5603474c9743fd04f3da888644413dc5" ->
> "ff2775f8d1dc753f635830adcc3a067e0b681e2d"; // ?
> }
> +%% The same graph, but excluding deps introduced by file.txt.
> +% guilt graph -x file.txt
> +digraph G {
> +# checking rev bc7df666a646739eaf559af23cab72f2bfd01f0e
> + "bc7df666a646739eaf559af23cab72f2bfd01f0e"
> [label="a-\"better&quicker'-patch.patch"]
> +# checking rev 891bc14b5603474c9743fd04f3da888644413dc5
> + "891bc14b5603474c9743fd04f3da888644413dc5" [label="c.patch"]
> +# checking rev c7014443c33d2b0237293687ceb9cbd38313df65
> + "c7014443c33d2b0237293687ceb9cbd38313df65" [label="b.patch"]
> +# checking rev ff2775f8d1dc753f635830adcc3a067e0b681e2d
> + "ff2775f8d1dc753f635830adcc3a067e0b681e2d" [label="a.patch"]
> +}
> diff --git a/regression/t-033.sh b/regression/t-033.sh
> index 9fe1827..ae22914 100755
> --- a/regression/t-033.sh
> +++ b/regression/t-033.sh
> @@ -59,3 +59,6 @@ cmd git add file.txt
> cmd guilt refresh
> fixup_time_info "a-\"better&quicker'-patch.patch"
> cmd guilt graph
> +
> +echo "%% The same graph, but excluding deps introduced by file.txt."
> +cmd guilt graph -x file.txt
> --
> 2.1.0
>
--
Computer Science is no more about computers than astronomy is about
telescopes.
- Edsger Dijkstra
--
To unsubscribe from this list: send the line "unsubscribe 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 compile warnings (under mac/clang)
Jeff King writes:
>> diff --git a/fsck.c b/fsck.c
>> index 15cb8bd..8f8c82f 100644
>> --- a/fsck.c
>> +++ b/fsck.c
>> @@ -107,7 +107,7 @@ static int fsck_msg_severity(enum fsck_msg_id msg_id,
>> {
>> int severity;
>>
>> -if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
>> +if (options->msg_severity && ((unsigned int) msg_id) < FSCK_MSG_MAX)
>> severity = options->msg_severity[msg_id];
>> else {
>> severity = msg_id_info[msg_id].severity;
>> -- snap --
>>
>> What do you think? Michael, does this cause more Clang warnings,
>> or would it resolve the issue?
>
> Hmm, yeah, that does not seem unreasonable, and is more localized.
Or we could force enum to be signed by defining FSCK_MSG_UNUSED to
be -1 at the very beginning of enum definition, without changing
anything else. Then "msg_id < 0" would become a very valid
protection against programming mistakes, 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 0/7] Coding style fixes.
On Fri, Jan 23, 2015 at 4:09 AM, Alexander Kuleshov wrote: > I made separate patch for every file. Please, let me know if need to > squash it into one commit. > > > 2015-01-23 17:06 GMT+06:00 Alexander Kuleshov : >> This patch set contatins minor style fixes. CodingGuidelines contains >> rule that the star must side with variable name. >> The whole series is Reviewed-by: Stefan Beller -- To unsubscribe from this list: send the line "unsubscribe 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 compile warnings (under mac/clang)
On Fri, Jan 23, 2015 at 10:07:18AM -0800, Junio C Hamano wrote:
> >> diff --git a/fsck.c b/fsck.c
> >> index 15cb8bd..8f8c82f 100644
> >> --- a/fsck.c
> >> +++ b/fsck.c
> >> @@ -107,7 +107,7 @@ static int fsck_msg_severity(enum fsck_msg_id msg_id,
> >> {
> >>int severity;
> >>
> >> - if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
> >> + if (options->msg_severity && ((unsigned int) msg_id) < FSCK_MSG_MAX)
> >>severity = options->msg_severity[msg_id];
> >>else {
> >>severity = msg_id_info[msg_id].severity;
> >> -- snap --
> >>
> >> What do you think? Michael, does this cause more Clang warnings,
> >> or would it resolve the issue?
> >
> > Hmm, yeah, that does not seem unreasonable, and is more localized.
>
> Or we could force enum to be signed by defining FSCK_MSG_UNUSED to
> be -1 at the very beginning of enum definition, without changing
> anything else. Then "msg_id < 0" would become a very valid
> protection against programming mistakes, no?
Yeah, I think that would work, too. It is a little unfortunate in the
sense that it actually makes things _worse_ from the perspective of the
type system. That is, in the current code if you assume that everyone
else has followed the type rules, then an fsck_msg_id you get definitely
is indexable into various arrays. But if you add in a sentinel value,
now you (in theory) have to check for the sentinel value everywhere.
I'm not sure if that matters in practice, though, if you are going to be
defensive against people misusing the enum system in the first place
(e.g., you are worried about them passing a random int and having it
produce a segfault, you have to do range checks either way).
But of all the options outlined, I think I'd much rather just see an
assert() for something that should never happen, rather than mixing it
into the logic.
In that vein, one thing that puzzles me is that the current code looks
like:
if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
severity = options->msg_severity[msg_id];
else {
severity = msg_id_info[msg_id].severity;
...
}
So if the severity override list given by "options" exists, _and_ if we
are in the enum range, then we use that. Otherwise, we dereference the
global list. But wouldn't an out-of-range condition have the exact same
problem dereferencing that global list?
IOW, should this really be:
if (msg_id < 0 || msg_id >= FSCK_MSG_MAX)
die("BUG: broken enum");
if (options->msg_severity)
severity = options->msg_severity[msg_id];
else
severity = msg_id_info[msg_id].severity;
? And then you can spell that first part as assert(), which I suspect
(but did not test) may shut up clang's warnings.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: make -M -C mean the same as -C -M
Mike Hommey writes:
> While -C implies -M, it is quite common to see both on example command lines
> here and there. The unintuitive thing is that if -M appears after -C, then
> copy detection is turned off because of how the command line arguments are
> handled.
This is deliberate, see below.
> Change this so that when both -C and -M appear, whatever their order, copy
> detection is on.
>
> Signed-off-by: Mike Hommey
> ---
>
> Interestingly, I even found mentions of -C -M in this order for benchmarks,
> on this very list (see 6555655.XSJ9EnW4BY@mako).
Aside from the general warning against taking everything you see on
this list as true, I think you are looking at an orange and talking
about an apple. $gmane/244581 is about "git blame", and "-M/-C"
over there mean completely different things.
Imagine you have a file a/b/c/d/e/f/g/h/, where each alphabet
represents a line with more meaningful contents than a single-liner,
and slash represents an end of line, and you have changed the
contents of the file to e/f/g/h/a/b/c/d/.
"blame -M" is to tell the command to notice that you moved the first
four lines to the end (i.e. you did not do anything original and
just moved lines). Because this needs more processing to notice,
the feature is triggered by a "-M" option (you would see something
like "e/f/g/h/ came from the original and then a/b/c/d/ are newly
added" without the option). "-M" in "blame" is about showing such
movement of lines within the same file [*1*].
On the other hand "-C" in "blame" is about noticing contents that
were copied from other files.
In the context of "git blame", "-C" and "-M" control orthogonal
concepts and it makes sense to use only one but not the other, or
both.
But "-M" given to "git diff" is not about such an orthogonal
concept.
Even though its source candidates are narrower than that of "-C" in
that "-M" chooses only from the set of files that disappeared while
"-C" also chooses from files that remain after the change, they are
both about "which file did the whole thing come from?", and it makes
sense to have progression of "-M" < "-C" < "-C -C". You can think
of these as a short-hand for --rename-copy-level={0,1,2,3} option
(where the level 0 -- do not do anything -- corresponds to giving no
"-M/-C"). It can be argued both ways: either we take the maximum of
the values given to --rename-copy-level options (which corresponds
to what your patch attempts to do), or the last one wins. We happen
to have implemented the latter long time ago and that is how
existing users expect things to work.
So, I dunno. I am saying that the semantics the current code gives
is *not* the only possibly correct one, and I am not saying that
your alternative interpretation is *wrong*, but I am not sure if it
makes sense to switch the semantics this late in the game.
[Footnote]
*1* "diff" cannot do anything magical about such a change. It can
only say that you removed the first four lines from the original,
and then added new four lines at the end (or it may say you added
new four lines at the beginning and removed four lines at the end).
There is no way for "diff" to say that these new four lines have any
relation to what you removed from the beginning of the file, with
any combination of options. This is inherent in its output format,
which is limited to express only deletions and additions.
> diff.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/diff.c b/diff.c
> index d1bd534..9081fd8 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3670,7 +3670,8 @@ int diff_opt_parse(struct diff_options *options, const
> char **av, int ac)
>!strcmp(arg, "--find-renames")) {
> if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
> return error("invalid argument to -M: %s", arg+2);
> - options->detect_rename = DIFF_DETECT_RENAME;
> + if (options->detect_rename != DIFF_DETECT_COPY)
> + options->detect_rename = DIFF_DETECT_RENAME;
> }
> else if (!strcmp(arg, "-D") || !strcmp(arg, "--irreversible-delete")) {
> options->irreversible_delete = 1;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git compile warnings (under mac/clang)
Hi Peff,
On 2015-01-23 19:37, Jeff King wrote:
> On Fri, Jan 23, 2015 at 10:07:18AM -0800, Junio C Hamano wrote:
>
> [...] one thing that puzzles me is that the current code looks
> like:
>
> if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
> severity = options->msg_severity[msg_id];
> else {
> severity = msg_id_info[msg_id].severity;
> ...
> }
>
> So if the severity override list given by "options" exists, _and_ if we
> are in the enum range, then we use that. Otherwise, we dereference the
> global list. But wouldn't an out-of-range condition have the exact same
> problem dereferencing that global list?
>
> IOW, should this really be:
>
> if (msg_id < 0 || msg_id >= FSCK_MSG_MAX)
> die("BUG: broken enum");
>
> if (options->msg_severity)
> severity = options->msg_severity[msg_id];
> else
> severity = msg_id_info[msg_id].severity;
>
> ? And then you can spell that first part as assert(), which I suspect
> (but did not test) may shut up clang's warnings.
To be quite honest, I assumed that Git's source code was assert()-free. But I
was wrong! So I'll go with that solution; it is by far the nicest one IMHO.
Thanks!
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git compile warnings (under mac/clang)
Jeff King writes:
> But of all the options outlined, I think I'd much rather just see an
> assert() for something that should never happen, rather than mixing it
> into the logic.
Surely.
> In that vein, one thing that puzzles me is that the current code looks
> like:
>
> if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
> severity = options->msg_severity[msg_id];
> else {
> severity = msg_id_info[msg_id].severity;
> ...
> }
>
> So if the severity override list given by "options" exists, _and_ if we
> are in the enum range, then we use that. Otherwise, we dereference the
> global list. But wouldn't an out-of-range condition have the exact same
> problem dereferencing that global list?
>
> IOW, should this really be:
>
> if (msg_id < 0 || msg_id >= FSCK_MSG_MAX)
> die("BUG: broken enum");
>
> if (options->msg_severity)
> severity = options->msg_severity[msg_id];
> else
> severity = msg_id_info[msg_id].severity;
>
> ? And then you can spell that first part as assert(), which I suspect
> (but did not test) may shut up clang's warnings.
Sounds like a sensible fix 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: Git compile warnings (under mac/clang)
On Fri, Jan 23, 2015 at 07:46:36PM +0100, Johannes Schindelin wrote:
> > ? And then you can spell that first part as assert(), which I suspect
> > (but did not test) may shut up clang's warnings.
>
> To be quite honest, I assumed that Git's source code was
> assert()-free. But I was wrong! So I'll go with that solution; it is
> by far the nicest one IMHO.
OK, here it is as a patch on top of js/fsck-opt. Please feel free to
squash rather than leaving it separate.
I tested with clang-3.6, and it seems to make the warning go away.
-- >8 --
Subject: [PATCH] fsck_msg_severity: range-check enum with assert()
An enum is passed into the function, which we use to index a
fixed-size array. We double-check that the enum is within
range as a defensive measure. However, there are two
problems with this:
1. If it's not in range, we then use it to index another
array of the same size. Which will have the same problem.
We should probably die instead, as this condition
should not ever happen.
2. The bottom end of our range check is tautological
according to clang, which generates a warning. Clang is
not _wrong_, but the point is that we are trying to be
defensive against something that should never happen
(i.e., somebody abusing the enum).
We can solve both by switching to a separate assert().
Signed-off-by: Jeff King
---
fsck.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fsck.c b/fsck.c
index 15cb8bd..53c0849 100644
--- a/fsck.c
+++ b/fsck.c
@@ -107,7 +107,9 @@ static int fsck_msg_severity(enum fsck_msg_id msg_id,
{
int severity;
- if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
+ assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);
+
+ if (options->msg_severity)
severity = options->msg_severity[msg_id];
else {
severity = msg_id_info[msg_id].severity;
--
2.3.0.rc1.287.g761fd19
--
To unsubscribe from this list: send the line "unsubscribe 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 compile warnings (under mac/clang)
Hi Peff,
On 2015-01-23 19:55, Jeff King wrote:
> On Fri, Jan 23, 2015 at 07:46:36PM +0100, Johannes Schindelin wrote:
>
>> > ? And then you can spell that first part as assert(), which I suspect
>> > (but did not test) may shut up clang's warnings.
>>
>> To be quite honest, I assumed that Git's source code was
>> assert()-free. But I was wrong! So I'll go with that solution; it is
>> by far the nicest one IMHO.
>
> OK, here it is as a patch on top of js/fsck-opt. Please feel free to
> squash rather than leaving it separate.
>
> I tested with clang-3.6, and it seems to make the warning go away.
>
> -- >8 --
> Subject: [PATCH] fsck_msg_severity: range-check enum with assert()
>
> An enum is passed into the function, which we use to index a
> fixed-size array. We double-check that the enum is within
> range as a defensive measure. However, there are two
> problems with this:
>
> 1. If it's not in range, we then use it to index another
> array of the same size. Which will have the same problem.
> We should probably die instead, as this condition
> should not ever happen.
>
> 2. The bottom end of our range check is tautological
> according to clang, which generates a warning. Clang is
> not _wrong_, but the point is that we are trying to be
> defensive against something that should never happen
> (i.e., somebody abusing the enum).
>
> We can solve both by switching to a separate assert().
>
> Signed-off-by: Jeff King
> ---
> fsck.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fsck.c b/fsck.c
> index 15cb8bd..53c0849 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -107,7 +107,9 @@ static int fsck_msg_severity(enum fsck_msg_id msg_id,
> {
> int severity;
>
> - if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
> + assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);
> +
> + if (options->msg_severity)
> severity = options->msg_severity[msg_id];
> else {
> severity = msg_id_info[msg_id].severity;
I also ended up with that solution!
Thanks!
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
git pull not ignoring the file which has been sent to the temporary ignore list
Hi, I asked git not to track any changes to the file .gitignore. To do so I did use the command - git update-index --assume-unchanged .gitignore. [arup@sztukajedzenia]$ git status # On branch MajorUpgrade # Your branch is behind 'origin/MajorUpgrade' by 4 commits, and can be fast-forwarded. # (use "git pull" to update your local branch) # nothing to commit, working directory clean [arup@sztukajedzenia]$ git pull origin MajorUpgrade >From rubyxcube.co.uk:sztuka-jedzenia/sztukajedzenia * branchMajorUpgrade -> FETCH_HEAD Updating 59a1c07..d7b9cd3 error: Your local changes to the following files would be overwritten by merge: .gitignore Please, commit your changes or stash them before you can merge. Aborting [arup@sztukajedzenia]$ But you can see above, while I am taking `pull`, it is considering the file .gitignore. How should I tell `git pull` also not to consider the change the file, and skip it or something else ? -- Regards, Arup Rakshit Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it. --Brian Kernighan -- To unsubscribe from this list: send the line "unsubscribe 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 pull not ignoring the file which has been sent to the temporary ignore list
Arup Rakshit writes: > I asked git not to track any changes to the file .gitignore. To do > so I did use the command - git update-index --assume-unchanged > .gitignore. You are not asking Git to do anything. You promised Git that you will make no changes to .gitignore, and then broke that promise. Assume-unchanged is *not* "Ignore changes to this path". -- To unsubscribe from this list: send the line "unsubscribe 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 pull not ignoring the file which has been sent to the temporary ignore list
On Friday, January 23, 2015 11:31:40 AM you wrote: > Arup Rakshit writes: > > > I asked git not to track any changes to the file .gitignore. To do > > so I did use the command - git update-index --assume-unchanged > > .gitignore. > > You are not asking Git to do anything. You promised Git that you > will make no changes to .gitignore, and then broke that promise. > > Assume-unchanged is *not* "Ignore changes to this path". Ok. How should I then ignore any local changes to the .gitignore file ? And while taking pull, git should skip this file ? -- Regards, Arup Rakshit Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it. --Brian Kernighan -- To unsubscribe from this list: send the line "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] Documentation: what does "git log --indexed-objects" even mean?
4fe10219 (rev-list: add --indexed-objects option, 2014-10-16) adds "--indexed-objects" option to "rev-list", and it is only useful in the context of "git rev-list" and not "git log". There are other object traversal options that do not make sense for "git log" that are shown in the manual page. Move the description of "--indexed-objects" to the object traversal section so that it sits together with its friends "--objects", "--objects-edge", etc. and then show them only in "git rev-list" documentation. Signed-off-by: Junio C Hamano --- Documentation/rev-list-options.txt | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 2984f40..97ef2e8 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -172,11 +172,6 @@ explicitly. Pretend as if all objects mentioned by reflogs are listed on the command line as ``. ---indexed-objects:: - Pretend as if all trees and blobs used by the index are listed - on the command line. Note that you probably want to use - `--objects`, too. - --ignore-missing:: Upon seeing an invalid object name in the input, pretend as if the bad input was not given. @@ -644,6 +639,7 @@ Object Traversal These options are mostly targeted for packing of Git repositories. +ifdef::git-rev-list[] --objects:: Print the object IDs of any object referenced by the listed commits. `--objects foo ^bar` thus means ``send me @@ -662,9 +658,15 @@ These options are mostly targeted for packing of Git repositories. commits at the cost of increased time. This is used instead of `--objects-edge` to build ``thin'' packs for shallow repositories. +--indexed-objects:: + Pretend as if all trees and blobs used by the index are listed + on the command line. Note that you probably want to use + `--objects`, too. + --unpacked:: Only useful with `--objects`; print the object IDs that are not in packs. +endif::git-rev-list[] --no-walk[=(sorted|unsorted)]:: Only show the given commits, but do not traverse their ancestors. -- To unsubscribe from this list: send the line "unsubscribe 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] Documentation: what does "git log --indexed-objects" even mean?
On Fri, Jan 23, 2015 at 11:49:05AM -0800, Junio C Hamano wrote: > 4fe10219 (rev-list: add --indexed-objects option, 2014-10-16) adds > "--indexed-objects" option to "rev-list", and it is only useful in > the context of "git rev-list" and not "git log". There are other > object traversal options that do not make sense for "git log" that > are shown in the manual page. > > Move the description of "--indexed-objects" to the object traversal > section so that it sits together with its friends "--objects", > "--objects-edge", etc. and then show them only in "git rev-list" > documentation. Yeah, that makes sense to me. Acked-by: Jeff King -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 0/6] Fix bug in large transactions
version3:
patches 1,2,3 stayed completely as is, while patches 4,5 are new, patch 6 is
rewritten to first write the contents of the lock files before closing them.
This combines the series "Enable large transactions v2" as sent out yesterday
with the follow up series "[RFC PATCH 0/5] So you dislike the sequence of
system calls?"
There is no write_in_full_to_lock_file wrapper any more, but write_ref_sha1
was reduced in functionality in patch 5.
This applies on top of origin/sb/atomic-push and results in a merge conflict
when merging it to origin/jk/lock-ref-sha1-basic-return-errors which looks
like
$git checkout origin/jk/lock-ref-sha1-basic-return-errors
$git merge enable_large_transactions
CONFLICT (content): Merge conflict in refs.c
$git diff
++<<< HEAD
+ lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
+ if (lock->lock_fd < 0) {
++===
+ if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
++>>> enable_large_transactions
which is best resolved as:
@@@ -2316,8 -2333,7 +2333,12 @@@ static struct ref_lock *lock_ref_sha1_b
goto error_return;
}
+if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
last_errno = errno;
if (errno == ENOENT && --attempts_remaining > 0)
/*
version2:
* This applies on top of origin/sb/atomic-push though it will result in a one
line merge conflict with origin/jk/lock-ref-sha1-basic-return-errors when
merging to origin/next.
* It now uses the FILE* pointer instead of file descriptors. This
results in a combination of the 2 former patches "refs.c: have
a write_in_full_to_lock_file wrapper" and "refs.c: write to a
lock file only once" as the wrapper function is more adapted to
its consumers
* no need to dance around with char *pointers which may leak.
* another new patch sneaked into the series: Renaming ULIMIT in t7004
to ULIMIT_STACK_SIZE
That said, only the first and third patch are updated from the first version
of the patches. The others are new in the sense that rewriting them was cheaper
than keeping notes in between.
version1:
(reported as: git update-ref --stdin : too many open files, 2014-12-20)
First a test case is introduced to demonstrate the failure,
the patches 2-6 are little refactoring and the last patch
fixes the bug and also marks the bugs as resolved in the
test suite.
Unfortunately this applies on top of origin/next.
Any feedback would be welcome!
Thanks,
Stefan
Stefan Beller (6):
update-ref: test handling large transactions properly
t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE
refs.c: remove lock_fd from struct ref_lock
refs.c: move static functions to close and commit refs
refs.c: remove unlock_ref and commit_ref from write_ref_sha1
refs.c: enable large transactions
refs.c| 93 +++
t/t1400-update-ref.sh | 28
t/t7004-tag.sh| 4 +--
3 files changed, 79 insertions(+), 46 deletions(-)
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 1/6] update-ref: test handling large transactions properly
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Notes:
v2->v3:
no changes
t/t1400-update-ref.sh | 28
1 file changed, 28 insertions(+)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 7b4707b..47d2fe9 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -973,4 +973,32 @@ test_expect_success 'stdin -z delete refs works with
packed and loose refs' '
test_must_fail git rev-parse --verify -q $c
'
+run_with_limited_open_files () {
+ (ulimit -n 32 && "$@")
+}
+
+test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
+
+test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating
branches does not burst open file limit' '
+(
+ for i in $(test_seq 33)
+ do
+ echo "create refs/heads/$i HEAD"
+ done >large_input &&
+ run_with_limited_open_files git update-ref --stdin large_input &&
+ run_with_limited_open_files git update-ref --stdin http://vger.kernel.org/majordomo-info.html
[PATCHv3 6/6] refs.c: enable large transactions
By closing the file descriptors after creating the lock file we are not
limiting the size of the transaction by the number of available file
descriptors.
When closing the file descriptors early, we also need to write the values
in early, if we don't want to reopen the files.
Signed-off-by: Stefan Beller
---
Notes:
version3:
* Do not reopen the files after closing them. Make sure we have
written all necessary information before closing the file.
Doing it that way enabled us to drop the patch "[PATCH 4/6]
refs.c: Have a write_in_full_to_lock_file wrapping write_in_full"
refs.c| 15 ---
t/t1400-update-ref.sh | 4 ++--
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 1bfc84b..2594b23 100644
--- a/refs.c
+++ b/refs.c
@@ -3752,6 +3752,17 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update->refname);
goto cleanup;
}
+ if (!is_null_sha1(update->new_sha1)) {
+ if (write_ref_sha1(update->lock, update->new_sha1,
+ update->msg)) {
+ strbuf_addf(err, "Cannot write to the ref lock
'%s'.",
+ update->refname);
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+ }
+ /* Do not keep all lock files open at the same time. */
+ close_ref(update->lock);
}
/* Perform updates first so live commits remain referenced */
@@ -3759,9 +3770,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update->new_sha1)) {
- if (write_ref_sha1(update->lock, update->new_sha1,
- update->msg)
- || commit_ref(update->lock, update->new_sha1)) {
+ if (commit_ref(update->lock, update->new_sha1)) {
strbuf_addf(err, "Cannot update the ref '%s'.",
update->refname);
ret = TRANSACTION_GENERIC_ERROR;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 47d2fe9..c593a1d 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -979,7 +979,7 @@ run_with_limited_open_files () {
test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating
branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating
branches does not burst open file limit' '
(
for i in $(test_seq 33)
do
@@ -990,7 +990,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large
transaction creating branches
)
'
-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting
branches does not burst open file limit' '
+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting
branches does not burst open file limit' '
(
for i in $(test_seq 33)
do
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 4/6] refs.c: move static functions to close and commit refs
By moving the functions up we don't need to have to declare them first
when using them in a later patch.
Signed-off-by: Stefan Beller
---
Notes:
new in v3
refs.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/refs.c b/refs.c
index 4066752..f1eefc7 100644
--- a/refs.c
+++ b/refs.c
@@ -2808,6 +2808,20 @@ static int rename_ref_available(const char *oldname,
const char *newname)
static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *logmsg);
+static int close_ref(struct ref_lock *lock)
+{
+ if (close_lock_file(lock->lk))
+ return -1;
+ return 0;
+}
+
+static int commit_ref(struct ref_lock *lock)
+{
+ if (commit_lock_file(lock->lk))
+ return -1;
+ return 0;
+}
+
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
unsigned char sha1[20], orig_sha1[20];
@@ -2898,20 +2912,6 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return 1;
}
-static int close_ref(struct ref_lock *lock)
-{
- if (close_lock_file(lock->lk))
- return -1;
- return 0;
-}
-
-static int commit_ref(struct ref_lock *lock)
-{
- if (commit_lock_file(lock->lk))
- return -1;
- return 0;
-}
-
/*
* copy the reflog message msg to buf, which has been allocated sufficiently
* large, while cleaning up the whitespaces. Especially, convert LF to space,
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
This makes write_ref_sha1 only write the the lock file, committing
needs to be done outside of that function. This will help us change
the ref_transaction_commit in a later patch.
Also instead of calling unlock_ref before each return in write_ref_sha1
we can call this after the call. This is a first step to split up
write_ref_sha1 into the write and commit phase which is done in a
later patch.
There is a call in each code path after write_ref_sha1 now. Even in
the last hunk in the error case, the 'goto cleanup;' will make sure
there is a call to unlock_ref.
Signed-off-by: Stefan Beller
---
Notes:
new in v3
refs.c | 40
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/refs.c b/refs.c
index f1eefc7..1bfc84b 100644
--- a/refs.c
+++ b/refs.c
@@ -2815,8 +2815,11 @@ static int close_ref(struct ref_lock *lock)
return 0;
}
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
{
+ if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
+ return 0;
+
if (commit_lock_file(lock->lk))
return -1;
return 0;
@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
lock->force_write = 1;
hashcpy(lock->old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+ if (write_ref_sha1(lock, orig_sha1, logmsg)
+ || commit_ref(lock, orig_sha1)) {
+ unlock_ref(lock);
error("unable to write current sha1 into %s", newrefname);
goto rollback;
}
+ unlock_ref(lock);
return 0;
@@ -2896,8 +2902,11 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
lock->force_write = 1;
flag = log_all_ref_updates;
log_all_ref_updates = 0;
- if (write_ref_sha1(lock, orig_sha1, NULL))
+ if (write_ref_sha1(lock, orig_sha1, NULL)
+ || commit_ref(lock, orig_sha1))
error("unable to write current sha1 into %s", oldrefname);
+
+ unlock_ref(lock);
log_all_ref_updates = flag;
rollbacklog:
@@ -3067,22 +3076,19 @@ static int write_ref_sha1(struct ref_lock *lock,
errno = EINVAL;
return -1;
}
- if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
- unlock_ref(lock);
+ if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
return 0;
- }
+
o = parse_object(sha1);
if (!o) {
error("Trying to write ref %s with nonexistent object %s",
lock->ref_name, sha1_to_hex(sha1));
- unlock_ref(lock);
errno = EINVAL;
return -1;
}
if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
error("Trying to write non-commit object %s to branch %s",
sha1_to_hex(sha1), lock->ref_name);
- unlock_ref(lock);
errno = EINVAL;
return -1;
}
@@ -3091,7 +3097,6 @@ static int write_ref_sha1(struct ref_lock *lock,
close_ref(lock) < 0) {
int save_errno = errno;
error("Couldn't write %s", lock->lk->filename.buf);
- unlock_ref(lock);
errno = save_errno;
return -1;
}
@@ -3099,7 +3104,6 @@ static int write_ref_sha1(struct ref_lock *lock,
if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
(strcmp(lock->ref_name, lock->orig_ref_name) &&
log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) <
0)) {
- unlock_ref(lock);
return -1;
}
if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
@@ -3124,12 +3128,6 @@ static int write_ref_sha1(struct ref_lock *lock,
!strcmp(head_ref, lock->ref_name))
log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
}
- if (commit_ref(lock)) {
- error("Couldn't set %s", lock->ref_name);
- unlock_ref(lock);
- return -1;
- }
- unlock_ref(lock);
return 0;
}
@@ -3762,14 +3760,15 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (!is_null_sha1(update->new_sha1)) {
if (write_ref_sha1(update->lock, update->new_sha1,
- update->msg)) {
- update->lock = NULL; /* freed by write_ref_sha1
*/
+ update->msg)
+ || commit_ref(update->lock, update->new_sha1)) {
strbuf_addf(err, "Cannot update the ref '%s'.",
update->refname
[PATCHv3 2/6] t7004: rename ULIMIT test prerequisite to ULIMIT_STACK_SIZE
During creation of the patch series our discussion we could have a
more descriptive name for the prerequisite for the test so it stays
unique when other limits of ulimit are introduced.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Notes:
v2->v3:
no changes
t/t7004-tag.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 796e9f7..06b8e0d 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1463,10 +1463,10 @@ run_with_limited_stack () {
(ulimit -s 128 && "$@")
}
-test_lazy_prereq ULIMIT 'run_with_limited_stack true'
+test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
# we require ulimit, this excludes Windows
-test_expect_success ULIMIT '--contains works in a deep repo' '
+test_expect_success ULIMIT_STACK_SIZE '--contains works in a deep repo' '
>expect &&
i=1 &&
while test $i -lt 8000
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 3/6] refs.c: remove lock_fd from struct ref_lock
The 'lock_fd' is the same as 'lk->fd'. No need to store it twice so remove
it. You may argue this introduces more coupling as we need to know more
about the internals of the lock file mechanism, but this will be solved in
a later patch.
No functional changes intended.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Notes:
v2->v3:
no changes
refs.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 14e52ca..4066752 100644
--- a/refs.c
+++ b/refs.c
@@ -11,7 +11,6 @@ struct ref_lock {
char *orig_ref_name;
struct lock_file *lk;
unsigned char old_sha1[20];
- int lock_fd;
int force_write;
};
@@ -2259,7 +2258,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
int attempts_remaining = 3;
lock = xcalloc(1, sizeof(struct ref_lock));
- lock->lock_fd = -1;
if (mustexist)
resolve_flags |= RESOLVE_REF_READING;
@@ -2335,8 +2333,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
goto error_return;
}
- lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
- if (lock->lock_fd < 0) {
+ if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
+ last_errno = errno;
if (errno == ENOENT && --attempts_remaining > 0)
/*
* Maybe somebody just deleted one of the
@@ -2904,7 +2902,6 @@ static int close_ref(struct ref_lock *lock)
{
if (close_lock_file(lock->lk))
return -1;
- lock->lock_fd = -1;
return 0;
}
@@ -2912,7 +2909,6 @@ static int commit_ref(struct ref_lock *lock)
{
if (commit_lock_file(lock->lk))
return -1;
- lock->lock_fd = -1;
return 0;
}
@@ -3090,8 +3086,8 @@ static int write_ref_sha1(struct ref_lock *lock,
errno = EINVAL;
return -1;
}
- if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
- write_in_full(lock->lock_fd, &term, 1) != 1 ||
+ if (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||
+ write_in_full(lock->lk->fd, &term, 1) != 1 ||
close_ref(lock) < 0) {
int save_errno = errno;
error("Couldn't write %s", lock->lk->filename.buf);
@@ -4047,9 +4043,9 @@ int reflog_expire(const char *refname, const unsigned
char *sha1,
status |= error("couldn't write %s: %s", log_file,
strerror(errno));
} else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
- (write_in_full(lock->lock_fd,
+ (write_in_full(lock->lk->fd,
sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-write_str_in_full(lock->lock_fd, "\n") != 1 ||
+write_str_in_full(lock->lk->fd, "\n") != 1 ||
close_ref(lock) < 0)) {
status |= error("couldn't write %s",
lock->lk->filename.buf);
--
2.2.1.62.g3f15098
--
To unsubscribe from this list: send the line "unsubscribe 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 pull not ignoring the file which has been sent to the temporary ignore list
On Fri, Jan 23, 2015 at 10:35 AM, Arup Rakshit wrote: > On Friday, January 23, 2015 11:31:40 AM you wrote: >> Arup Rakshit writes: >> >> > I asked git not to track any changes to the file .gitignore. To do >> > so I did use the command - git update-index --assume-unchanged >> > .gitignore. >> >> You are not asking Git to do anything. You promised Git that you >> will make no changes to .gitignore, and then broke that promise. >> >> Assume-unchanged is *not* "Ignore changes to this path". > > Ok. How should I then ignore any local changes to the .gitignore file ? And > while taking pull, git should skip this file ? Look at .git/info/exclude When looking for a reference to that path (I am bad at remembering which man page that is) I found https://help.github.com/articles/ignoring-files/ as Googles first hit, which advises to use git update-index --assume-unchanged path/to/file.txt Not sure if that is most helpful advice there. See http://git-scm.com/docs/gitignore instead -- To unsubscribe from this list: send the line "unsubscribe 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 pull not ignoring the file which has been sent to the temporary ignore list
Stefan Beller writes: >> Ok. How should I then ignore any local changes to the .gitignore >> file ? And while taking pull, git should skip this file ? > > Look at .git/info/exclude Good answer for ".gitignore". In general, you do not "ignore local changes" to tracked paths. > I found https://help.github.com/articles/ignoring-files/ as Googles > first hit, which advises to use > git update-index --assume-unchanged path/to/file.txt > Not sure if that is most helpful advice there. The piece of advice in the last paragraph on that page is wrong (and it has been wrong from the day it was written). The gitignore(5) documentation used to have a similar incorrect piece of advice but we finally corrected it recently. Cf. http://thread.gmane.org/gmane.comp.version-control.git/260954/focus=261118 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
On 2015-01-22 23.07, Junio C Hamano wrote:
> Torsten Bögershausen writes:
>
>> If I run that sequence manually:
>> chmod 755 .
>> touch x
>> chmod a-w .
>> rm x
>> touch y
>>
>> x is gone, (but shoudn't according to POSIX)
>> y is not created, "access denied"
>
> Good (or is that Sad?).
>
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1039,7 +1039,17 @@ test_lazy_prereq NOT_ROOT '
>> # When the tests are run as root, permission tests will report that
>> # things are writable when they shouldn't be.
>> test_lazy_prereq SANITY '
>> - test_have_prereq POSIXPERM,NOT_ROOT
>> + mkdir ds &&
>> + touch ds/x &&
>> + chmod -w ds &&
>> + if rm ds/x
>> + then
>> + chmod +w ds
>> + false
>> + else
>> + chmod +w ds
>> + true
>> + fi
>> '
>
> It looks like a better approach overall.
>
> Because we cannot know where $(pwd) is when lazy prereq is evaluated
> (it typically is at the root of the trash hierarchy, but not always)
> and we would not want to add, leave or remove random files in the
> working tree that are not expected by the tests proper (e.g. a test
> that counts untracked paths are not expecting ds/ to be there), your
> actual "fix" may need to be a bit more careful, though.
>
> Thanks.
>
Hm,
I think there are 2 different possiblities to go further,
either to always switch off SANITY for CYGWIN (or Windows in general).
I haven't tested anything, the idea came up while writing this email.
The other way is to go away from the hard coded "we know we are root,
so SANITY must be false, or we know that Windows is not 100% POSIX",
and probe the OS/FS dynamically.
The following probably deserves the price for the most clumsy prerequisite
ever written.
(Copy&Paste of a real patch into the mailer, not sure if it applies)
It has been tested under Mac OS, root@Mac OS, Cygwin / Msysgit
What do you think ?
-- >8 --
Subject: [PATCH 1/2] test-lib.sh: Improve SANITY
SANITY was not set when running as root,
but this is not 100% reliable for CYGWIN:
A file is allowed to be deleted when the containing
directory does not have write permissions.
Signed-off-by: Torsten Bögershausen
---
t/test-lib.sh | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 93f7cad..b8f736f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1038,8 +1038,26 @@ test_lazy_prereq NOT_ROOT '
# When the tests are run as root, permission tests will report that
# things are writable when they shouldn't be.
+# Special check for CYGWIN (or Windows in general):
+# A file can be deleted, even if the containing directory does'nt
+# have write permissions
test_lazy_prereq SANITY '
- test_have_prereq POSIXPERM,NOT_ROOT
+ dsdir=$$ds
+ mkdir $dsdir &&
+ touch $dsdir/x &&
+ chmod -w $dsdir &&
+ if rm $dsdir/x
+ then
+ chmod +w $dsdir
+ rm -rf $dsdir
+ echo >&2 SANITY=false
+ false
+ else
+ chmod +w $dsdir
+ rm -rf $dsdir
+ echo >&2 SANITY=true
+ true
+ fi
'
GIT_UNZIP=${GIT_UNZIP:-unzip}
--
Subject: [PATCH 2/2] t2026 needs SANITY
When running as root 'prune directories with unreadable gitdir' in t2026 fails.
Protect this TC with SANITY
Signed-off-by: Torsten Bögershausen
---
t/t2026-prune-linked-checkouts.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t2026-prune-linked-checkouts.sh
b/t/t2026-prune-linked-checkouts.sh
index 170aefe..2936d52 100755
--- a/t/t2026-prune-linked-checkouts.sh
+++ b/t/t2026-prune-linked-checkouts.sh
@@ -33,7 +33,7 @@ EOF
! test -d .git/worktrees
'
-test_expect_success POSIXPERM 'prune directories with unreadable gitdir' '
+test_expect_success SANITY 'prune directories with unreadable gitdir' '
mkdir -p .git/worktrees/def/abc &&
: >.git/worktrees/def/def &&
: >.git/worktrees/def/gitdir &&
--
To unsubscribe from this list: send the line "unsubscribe 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 pull not ignoring the file which has been sent to the temporary ignore list
On Fri, Jan 23, 2015 at 1:14 PM, Junio C Hamano wrote: > > Good answer for ".gitignore". In general, you do not "ignore local > changes" to tracked paths. > I assumed Arup would want to ignore more than is in the upstream project, so you'd come up with an appendix to the .gitignore file because that file is rather obvious to find (it's printed when git pull modifies it, 'ls' just find it, you'd not look into .git/info/exclude by chance) Assuming you want to ignore less than the upstream project (delete some lines from .gitignore) it get's tricky in my opinion. Either have a local commit and just use 'git pull' to resolve the conflicts with upstream. The problem then arises if you want to publish your changes (such as pushing your changes and creating a pull request, then you have that commit included, which you maybe don't want to include) Mind, that I am talking about possible work flows to circumvent an assumed problem which would result in problems as described in the first mail. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-p4: correct --prepare-p4-only instructions
l...@diamand.org wrote on Fri, 23 Jan 2015 09:15 +: > If you use git-p4 with the "--prepare-p4-only" option, then > it prints the p4 command line to use. However, the command > line was incorrect: the changelist specification must be > supplied on standard input, not as an argument to p4. > > Signed-off-by: Luke Diamand > --- > git-p4.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-p4.py b/git-p4.py > index ff132b2..90447de 100755 > --- a/git-p4.py > +++ b/git-p4.py > @@ -1442,7 +1442,7 @@ class P4Submit(Command, P4UserMap): > print " " + self.clientPath > print > print "To submit, use \"p4 submit\" to write a new description," > -print "or \"p4 submit -i %s\" to use the one prepared by" \ > +print "or \"p4 submit -i <%s\" to use the one prepared by" \ >" \"git p4\"." % fileName > print "You can delete the file \"%s\" when finished." % fileName > Looks obviously good here. Ack! -- Pete -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git-p4 maintainership change
Hi Junio. I'm fortunate enough to need no longer any git integration with Perforce (p4). I work only in git these days. Thus you might expect my interest in improving git-p4 would be waning. Luke, on the other hand, continues to need git-p4 and is active in improving it. I think you should consider accepting patches in that area from him directly. He's contributed many patches over the years and has helped users to debug their issues too. I'll certainly be available to comment on any dodgy code in there already, and can help with archeological, but will not likely do any substantive work to git-p4 in the near future. Luke: I think you have a couple patches outstanding; hope these can get merged to mainline soon. Thanks, -- Pete -- To unsubscribe from this list: send the line "unsubscribe 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 --recurse-submodule does not recurse to sub-submodules (etc.)
Thanks, Jens. Incidentally, git submodule update --init --recursive Does exactly what expected – it updates sub/sub/submodules, so there is certainly some inconsistency in how the --recursive flag is handled here. i...@maxheld.de | http://www.maxheld.de | http://www.civicon.de | Mobil: +49 151 22958775 | Skype: maximilian.held Bremen International Graduate School of Social Sciences | Wiener Straße / Celsiusstraße | 28359 Bremen | Germany On Tue, Jan 20, 2015 at 10:21 PM, Jens Lehmann wrote: > Am 19.01.2015 um 21:19 schrieb Maximilian Held: > >> I have a directory with nested submodules, such as: >> >> supermodule/submodule/sub-submodule/sub-sub-submodule >> >> When I cd to supermodule and do: >> >> "git push --recurse-submodule=check" (or on-demand), >> >> git only pushes the submodule, but not the sub-submodule etc. >> >> Maybe this is expected behavior and not a bug, but I thought it was >> pretty unintuitive. I expected that git would push, well, recursively. > > > I agree this is unexpected and should be fixed. I suspect the fix > would be to teach the push_submodule() function to use the same > flags that were used for the push in the superproject. -- To unsubscribe from this list: send the line "unsubscribe 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 submodule first time update with proxy
Hi, On Fri, Jan 23, 2015 at 3:50 PM, Robert Dailey wrote: > I have a submodule using HTTP URL. I do this: > > $ git submodule init MySubmodule > $ git submodule update MySubmodule > > The 2nd command fails because the HTTP URL cannot be resolved, this is > because it requires a proxy. I have "http.proxy" setup properly in the > .git/config of my parent git repository, so I was hoping the submodule > update function would have a way to specify it to inherit the proxy > value from the parent config. Your not the first to suggest it and you probably won't be the last. It is hard to decide _which_ config variables, if any, should propagate from the parent. What works for one use-case may not necessarily work for another. > How can I set up my submodule? Probably the easiest thing would be to make your http.proxy configuration global i.e. $ git config --global http.proxy If you don't want to make it a global setting you can setup the submodule configuration after running init but before running update i.e. $ git submodule init MySubmodule $ (cd MySubmodule && git config http.proxy ...) $ git submodule update MySubmodule -- To unsubscribe from this list: send the line "unsubscribe 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 pull not ignoring the file which has been sent to the temporary ignore list
Stefan Beller writes: > Assuming you want to ignore less than the upstream project (delete some > lines from .gitignore) it get's tricky in my opinion. Why? Doesn't info/exclude allow negative ignore patterns? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: make -M -C mean the same as -C -M
On Fri, Jan 23, 2015 at 10:41:10AM -0800, Junio C Hamano wrote:
> Mike Hommey writes:
>
> > While -C implies -M, it is quite common to see both on example command lines
> > here and there. The unintuitive thing is that if -M appears after -C, then
> > copy detection is turned off because of how the command line arguments are
> > handled.
>
> This is deliberate, see below.
>
> > Change this so that when both -C and -M appear, whatever their order, copy
> > detection is on.
> >
> > Signed-off-by: Mike Hommey
> > ---
> >
> > Interestingly, I even found mentions of -C -M in this order for benchmarks,
> > on this very list (see 6555655.XSJ9EnW4BY@mako).
>
> Aside from the general warning against taking everything you see on
> this list as true
The problem is not whether what is on the list is true or not, but that
people will use what they see.
> I think you are looking at an orange and talking
> about an apple. $gmane/244581 is about "git blame", and "-M/-C"
> over there mean completely different things.
I thought blame was using the diff option parser, and I was wrong.
(snip)
> In the context of "git blame", "-C" and "-M" control orthogonal
> concepts and it makes sense to use only one but not the other, or
> both.
In the context of blame both -C and -M |= a flags set, so one doesn't
override the other. You can place them in any order, the result will be
the same. Incidentally, -C includes the flag -M sets, so -C -M is
actually redundant. What -C and -M can be used for is set different
scores (-C9 -M8).
> But "-M" given to "git diff" is not about such an orthogonal
> concept.
>
> Even though its source candidates are narrower than that of "-C" in
> that "-M" chooses only from the set of files that disappeared while
> "-C" also chooses from files that remain after the change, they are
> both about "which file did the whole thing come from?", and it makes
> sense to have progression of "-M" < "-C" < "-C -C". You can think
> of these as a short-hand for --rename-copy-level={0,1,2,3} option
> (where the level 0 -- do not do anything -- corresponds to giving no
> "-M/-C"). It can be argued both ways: either we take the maximum of
> the values given to --rename-copy-level options (which corresponds
> to what your patch attempts to do), or the last one wins. We happen
> to have implemented the latter long time ago and that is how
> existing users expect things to work.
Are they? I, for one, wasn't. It is easy to understand that --foo=1
--foo=2 is going to take the last given --foo, but do people really
expect the last of -C -M to be used? Reading the code further, I can see
that it's even more confusing than that: -C -C wins over -M, wherever it
happens. So -C -C -M == -C -C ; -C -M == -M ; -M -C == -C.
At the very least, this should be spelled out in the documentation.
Mike
--
To unsubscribe from this list: send the line "unsubscribe 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 pull not ignoring the file which has been sent to the temporary ignore list
On Fri, Jan 23, 2015 at 2:26 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> Assuming you want to ignore less than the upstream project (delete some >> lines from .gitignore) it get's tricky in my opinion. > > Why? Doesn't info/exclude allow negative ignore patterns? I used negative patterns only once, so they did not come to my mind today. Apologies for not looking it up before replying. :( -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff: make -M -C mean the same as -C -M
Mike Hommey writes: >> In the context of "git blame", "-C" and "-M" control orthogonal >> concepts and it makes sense to use only one but not the other, or >> both. > > In the context of blame both -C and -M |= a flags set, so one doesn't > override the other. You can place them in any order, the result will be > the same. Incidentally, -C includes the flag -M sets, so -C -M is > actually redundant. What -C and -M can be used for is set different > scores (-C9 -M8). Yes. That is what I meant by "orthogonal" and "makese sense to use only one but not the other, or both". As they are not related with each other, it makes sense to mix them freely, unlike "-C/-M" given to diff. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Should copy/rename detection consider file overwrites?
On Fri, Jan 23, 2015 at 06:04:19AM -0500, Jeff King wrote: > On Fri, Jan 23, 2015 at 10:29:08AM +0900, Mike Hommey wrote: > > > While fooling around with copy/rename detection, I noticed that it > > doesn't detect the case where you copy or rename a file on top of > > another: > > > > $ git init > > $ (echo foo; echo bar) > foo > > If I replace this with a longer input, like: > > cp /usr/share/dict/words foo > > > $ git add foo > > $ git commit -m foo > > $ echo 0 > bar > > $ git add bar > > $ git commit -m bar > > $ git mv -f foo bar > > $ git commit -m foobar > > $ git log --oneline --reverse > > 7dc2765 foo > > b0c837d bar > > 88caeba foobar > > $ git blame -s -C -C bar > > 88caebab 1) foo > > 88caebab 2) bar > > Then the blame shows me the initial "foo" commit. So I think it is > mainly that your toy example is too small (I think we will do > exact rename detection whatever the size is, but I expect we are getting > hung up on the break detection between "0\n" and "foo\nbar\n"). Err, I was afraid my testcase was too small. And that all boils down to this: is optional but it is the lower bound on the number of alphanumeric characters that Git must detect as moving/copying between files for it to associate those lines with the parent commit. And the default value is 40. > > I can see how this is not trivially representable in e.g. git diff-tree, > > but shouldn't at least blame try to tell that those lines actually come > > from 7dc2765? > > diff-tree can show this, too, but you need to turn on "break detection" > which will notice that "bar" has essentially been rewritten (and then > consider its sides as candidates for rename detection). For example > (with the longer input, as above): > > $ git diff-tree --name-status -M HEAD > c6fe146b0c73adcbc4dbc2e58eb83af9007678bc > M bar > D foo > > $ git diff-tree --name-status -M -B HEAD > c6fe146b0c73adcbc4dbc2e58eb83af9007678bc > R100foo bar > > Presumably if you set the break score low enough, your original example > would behave the same way, but I couldn't get it to work (I didn't look > closely, but I imagine it is just so tiny that we hit the internal > limits on how low you can set the score).o Oh. Good to know, thanks. Mike -- To unsubscribe from this list: send the line "unsubscribe 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-p4 maintainership change
Pete Wyckoff writes: > Hi Junio. I'm fortunate enough to need no longer any git > integration with Perforce (p4). I work only in git these days. > Thus you might expect my interest in improving git-p4 would > be waning. > > Luke, on the other hand, continues to need git-p4 and is > active in improving it. I think you should consider accepting > patches in that area from him directly. He's contributed many > patches over the years and has helped users to debug their issues > too. > > I'll certainly be available to comment on any dodgy code in there > already, and can help with archeological, but will not likely do > any substantive work to git-p4 in the near future. Thanks for your help during all these years, and thanks Luke for taking the area maintainership. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH] t/lib-httpd: switch SANITY check for NOT_ROOT
Torsten Bögershausen writes:
> It has been tested under Mac OS, root@Mac OS, Cygwin / Msysgit
> What do you think ?
Except that we may want to be more careful to detect errors from the
initial mkdir and clean-up part (which should abort the test, not
just declare !SANITY), I think the basic idea is sound.
test_dir=$TRASH_DIRECTORY/.sanity-test-dir
! mkdir "$test_dir" &&
>"$test_dir/x" &&
chmod -w "$test_dir" ||
error "bug in test sript: cannot prepare .sanity-test-dir"
rm "$test_dir/x"
status=$?
chmod +w "$test_dir" &&
rm -r "$test_dir" ||
error "bug in test sript: cannot clean .sanity-test-dir"
return $status
or something along that line?
>
> -- >8 --
> Subject: [PATCH 1/2] test-lib.sh: Improve SANITY
>
> SANITY was not set when running as root,
> but this is not 100% reliable for CYGWIN:
>
> A file is allowed to be deleted when the containing
> directory does not have write permissions.
>
> Signed-off-by: Torsten Bögershausen
> ---
> t/test-lib.sh | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 93f7cad..b8f736f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1038,8 +1038,26 @@ test_lazy_prereq NOT_ROOT '
>
> # When the tests are run as root, permission tests will report that
> # things are writable when they shouldn't be.
> +# Special check for CYGWIN (or Windows in general):
> +# A file can be deleted, even if the containing directory does'nt
> +# have write permissions
> test_lazy_prereq SANITY '
> - test_have_prereq POSIXPERM,NOT_ROOT
> + dsdir=$$ds
> + mkdir $dsdir &&
> + touch $dsdir/x &&
> + chmod -w $dsdir &&
> + if rm $dsdir/x
> + then
> + chmod +w $dsdir
> + rm -rf $dsdir
> + echo >&2 SANITY=false
> + false
> + else
> + chmod +w $dsdir
> + rm -rf $dsdir
> + echo >&2 SANITY=true
> + true
> + fi
> '
>
> GIT_UNZIP=${GIT_UNZIP:-unzip}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[ANNOUNCE] Git Merge, April 8-9, Paris
GitHub is organizing a Git-related conference to be held April 8-9, 2015, in Paris. Details here: http://git-merge.com/ The exact schedule is still being worked out, but there is going to be some dedicated time/space for Git (and libgit2 and JGit) developers to meet and talk to each other. If you have patches in Git, I'd encourage you to consider attending. If travel finances are a problem, please talk to me. GitHub may be able to defray the cost of travel. I hope to see people there! -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
Stefan Beller writes:
> -static int commit_ref(struct ref_lock *lock)
> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
> {
> + if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
> + return 0;
> if (commit_lock_file(lock->lk))
> return -1;
> return 0;
> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
> *newrefname, const char *logms
> }
> lock->force_write = 1;
> hashcpy(lock->old_sha1, orig_sha1);
> - if (write_ref_sha1(lock, orig_sha1, logmsg)) {
> + if (write_ref_sha1(lock, orig_sha1, logmsg)
> + || commit_ref(lock, orig_sha1)) {
> + unlock_ref(lock);
This is not a new problem, but the two lines in pre-context of this
patch look strange. When the code is renaming into some ref, the
ref either would have no original SHA-1 (i.e. we are renaming to a
non-existing name) or have unrelated SHA-1 (i.e. we are overwriting
an existing one). For some (unknown to me) reason, however, the
code pretends that lock->old_sha1 has the new SHA-1 already before
we start to do the write or commit.
And because both write and commit tries to pretend to be no-op when
the caller tries to update a ref with the same SHA-1, but in this
codepath it does want the write to happen, it needs to set the
force_write bit set, which look like an unnecessary workaround.
Regardless of what this particular caller does, I am not sure if the
early-return codepath in commit_ref() is correct. From the callers'
point of view, it sometimes unlocks the ref (i.e. when a different
SHA-1 is written or force_write is set) and sometimes keeps the ref
locked (i.e. when early-return is taken). Shouldn't these two cases
behave identically? Or am I wrong to assume that the early return
using "hashcmp(lock->old_sha1, sha1)" is a mere optimization?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 6/6] refs.c: enable large transactions
Stefan Beller writes: > By closing the file descriptors after creating the lock file we are not > limiting the size of the transaction by the number of available file > descriptors. > > When closing the file descriptors early, we also need to write the values > in early, if we don't want to reopen the files. I am not sure if an early return in write_ref_sha1() is entirely correct. The unlock step was split out of write and commit functions in the previous step because you eventually want to be able to close the file descriptor that is open to $ref.lock early, while still keeping the $ref.lock file around to avoid others competing with you, so that you can limit the number of open file descriptors, no? If so, shouldn't the write function at least close the file descriptor even when it knows that the $ref.lock already has the correct object name? The call to close_ref() is never made when the early-return codepath is taken as far as I can see. 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: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> -static int commit_ref(struct ref_lock *lock)
>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
>> {
>> + if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>> + return 0;
>> if (commit_lock_file(lock->lk))
>> return -1;
>> return 0;
>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
>> *newrefname, const char *logms
>> }
>> lock->force_write = 1;
>> hashcpy(lock->old_sha1, orig_sha1);
>> - if (write_ref_sha1(lock, orig_sha1, logmsg)) {
>> + if (write_ref_sha1(lock, orig_sha1, logmsg)
>> + || commit_ref(lock, orig_sha1)) {
>> + unlock_ref(lock);
>
> This is not a new problem, but the two lines in pre-context of this
> patch look strange.
Which (not new) problem are you talking about here? Do you have
a reference?
> Regardless of what this particular caller does, I am not sure if the
> early-return codepath in commit_ref() is correct. From the callers'
> point of view, it sometimes unlocks the ref (i.e. when a different
> SHA-1 is written or force_write is set) and sometimes keeps the ref
> locked (i.e. when early-return is taken). Shouldn't these two cases
> behave identically? Or am I wrong to assume that the early return
> using "hashcmp(lock->old_sha1, sha1)" is a mere optimization?
>
I assumed it was not just an optimization as the test suite fails if I
touch that line. I'll look into cleaning it up in a more obvious fashion.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 6/6] refs.c: enable large transactions
On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> By closing the file descriptors after creating the lock file we are not >> limiting the size of the transaction by the number of available file >> descriptors. >> >> When closing the file descriptors early, we also need to write the values >> in early, if we don't want to reopen the files. > > > I am not sure if an early return in write_ref_sha1() is entirely > correct. The unlock step was split out of write and commit > functions in the previous step because you eventually want to be > able to close the file descriptor that is open to $ref.lock early, > while still keeping the $ref.lock file around to avoid others > competing with you, so that you can limit the number of open file > descriptors, no? yeah that's the goal. Though as we're in one transaction, as soon as we have an early exit, the transaction will abort. > > If so, shouldn't the write function at least close the file > descriptor even when it knows that the $ref.lock already has the > correct object name? The call to close_ref() is never made when the > early-return codepath is taken as far as I can see. > The goto cleanup; will take care of unlocking (and closing fds of) all refs -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 6/6] refs.c: enable large transactions
Stefan Beller writes:
> On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano wrote:
>
> yeah that's the goal. Though as we're in one transaction, as soon
> as we have an early exit, the transaction will abort.
An early exit I am talking about is this:
static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
struct object *o;
if (!lock) {
errno = EINVAL;
return -1;
}
if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
return 0;
It returns 0 and then the transaction side has this call in a loop:
if (!is_null_sha1(update->new_sha1)) {
if (write_ref_sha1(update->lock, update->new_sha1,
update->msg)) {
strbuf_addf(err, "Cannot write to the ref lock
'%s'.",
update->refname);
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
}
>> If so, shouldn't the write function at least close the file
>> descriptor even when it knows that the $ref.lock already has the
>> correct object name? The call to close_ref() is never made when the
>> early-return codepath is taken as far as I can see.
>
> The goto cleanup; will take care of unlocking (and closing fds of) all refs
Yes, if write_ref_sha1() returns with non-zero signaling an error,
then the goto will trigger.
But if it short-cuts and returns zero, that goto will not be
reached.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
Stefan Beller writes:
> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano wrote:
>> Stefan Beller writes:
>>
>>> -static int commit_ref(struct ref_lock *lock)
>>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
>>> {
>>> + if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>>> + return 0;
>>> if (commit_lock_file(lock->lk))
>>> return -1;
>>> return 0;
>>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
>>> *newrefname, const char *logms
>>> }
>>> lock->force_write = 1;
>>> hashcpy(lock->old_sha1, orig_sha1);
>>> - if (write_ref_sha1(lock, orig_sha1, logmsg)) {
>>> + if (write_ref_sha1(lock, orig_sha1, logmsg)
>>> + || commit_ref(lock, orig_sha1)) {
>>> + unlock_ref(lock);
>>
>> This is not a new problem, but the two lines in pre-context of this
>> patch look strange.
>
> Which (not new) problem are you talking about here? Do you have
> a reference?
These two lines in pre-context of the hunk:
>>> lock->force_write = 1;
>>> hashcpy(lock->old_sha1, orig_sha1);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-new-workdir: support submodules
Ping! (now that the holidays are past) craig On Tue, Dec 23, 2014 at 1:51 PM, Craig Silverstein wrote: > [Ack, I forgot to cc myself on the original patch so now I can't reply > to it normally. Hopefully my workaround doesn't mess up the threading > too badly.] > > Junio C Hamano pobox.com> writes: >> >> H, does that mean that the submodule S in the original >> repository O's working tree and its checkout in the secondary >> working tree W created from O using git-new-workdir share the same >> repository location? More specifically: >> >> O/.git/ - original repository >> O/.git/index- worktree state in O >> O/S - submodule S's checkout in O >> O/S/.git- a gitfile pointing to O/.git/modules/S >> O/.git/modules/S- submodule S's repository contents >> O/.git/modules/S/config - submodule S's config >> >> W/.git/ - secondary working tree >> W/.git/config - symlink to O/.git/config >> W/.git/index- worktree state in W (independent of O) >> W/S - submodule S's checkout in W (independent of O) >> W/S/.git- a gitfile pointing to O/.git/modules/S > > Right until the last line. The .git file holds a relative path (at > least, it always does in my experience), so W/S/.git will point to > W/.git/modules/S. > > Also, to complete the story, my changes sets the following: > > W/.git/modules/S- secondary working tree for S > W/.git/modules/S/config - symlink to O/.git/modules/S/config > W/.git/modules/S/index- worktree state in W's S > (independent of O and O's S) > >> Doesn't a submodule checkout keep some state tied to the working >> tree in its repository configuration file? > > Do you mean, in 'config' itself? If so, I don't see it. (Though it's > possible there are ways to use submodules that do keep working-tree > state in the config file, and we just happen not to use those ways.) > Here's what my webapp/.git/modules/khan-exercises/config looks like: > --- > [core] > repositoryformatversion = 0 > filemode = true > bare = false > logallrefupdates = true > worktree = ../../../khan-exercises > [remote "origin"] > url = http://github.com/Khan/khan-exercises.git > fetch = +refs/heads/*:refs/remotes/origin/* > [branch "master"] > remote = origin > merge = refs/heads/master > rebase = true > [submodule "test/qunit"] > url = https://github.com/jquery/qunit.git > -- > > The only thing that seems vaguely working-tree related is the > 'worktree' field, which is the field that motivated me to set up my > patch the way it is. > >> Wouldn't this change >> introduce problems by sharing O/.git/modules/S/config between the >> two checkouts? > > It's true that this change does result in sharing that file, so if > that's problematic then you're right. I'm afraid I don't know all the > things that can go into a submodule config file. > > (There are other things I don't know as well, such as: do we see .git > files with 'gitdir: ...' contents only for submodules, or are there > other ways to create them as well? Are 'gitdir' paths always > relative? Are there special files in .git (or rather .git/modules/S) > that exist only for submodules and not for 'normal' repos, that we > need to worry about symlinking? I apologize for not knowing all these > git internals, and hope you guys can help point out any gaps that > affect this patch!) > > craig -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano wrote:
>>> Stefan Beller writes:
>>>
-static int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
{
+ if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
+ return 0;
if (commit_lock_file(lock->lk))
return -1;
return 0;
@@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
}
lock->force_write = 1;
hashcpy(lock->old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+ if (write_ref_sha1(lock, orig_sha1, logmsg)
+ || commit_ref(lock, orig_sha1)) {
+ unlock_ref(lock);
>>>
>>> This is not a new problem, but the two lines in pre-context of this
>>> patch look strange.
>>
>> Which (not new) problem are you talking about here? Do you have
>> a reference?
>
> These two lines in pre-context of the hunk:
>
lock->force_write = 1;
hashcpy(lock->old_sha1, orig_sha1);
>
So these 2 lines (specially the force_write=1 line) is just there to trigger
the valid early exit path as you sent in the other mail :
> if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
> return 0;
when we have the same sha1?
and you're saying that's a problem because hard to understand?
I am confused as well by now.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
Stefan Beller writes:
> On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano wrote:
>> Stefan Beller writes:
>>
>>> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano wrote:
Stefan Beller writes:
> -static int commit_ref(struct ref_lock *lock)
> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
> {
> + if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
> + return 0;
> if (commit_lock_file(lock->lk))
> return -1;
> return 0;
> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char
> *newrefname, const char *logms
> }
> lock->force_write = 1;
> hashcpy(lock->old_sha1, orig_sha1);
> - if (write_ref_sha1(lock, orig_sha1, logmsg)) {
> + if (write_ref_sha1(lock, orig_sha1, logmsg)
> + || commit_ref(lock, orig_sha1)) {
> + unlock_ref(lock);
This is not a new problem, but the two lines in pre-context of this
patch look strange.
>>>
>>> Which (not new) problem are you talking about here? Do you have
>>> a reference?
>>
>> These two lines in pre-context of the hunk:
>>
> lock->force_write = 1;
> hashcpy(lock->old_sha1, orig_sha1);
>>
>
> So these 2 lines (specially the force_write=1 line) is just there to trigger
> the valid early exit path as you sent in the other mail :
>
>> if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>> return 0;
>
> when we have the same sha1?
> and you're saying that's a problem because hard to understand?
What is the point of that hashcmp() in the first place? My
understanding is that
(1) lock->old_sha1 is to hold the original SHA-1 in the ref we took
the lock on.
(2) when not forcing, and when the two SHA-1 are the same, we
bypass and return early because overwriting the ref with the
same value is no-op.
Now, in that codepath, when the code is renaming into some ref, the
ref either would have no original SHA-1 (i.e. we are renaming to a
non-existing name) or have unrelated SHA-1 (i.e. we are overwriting
an existing one). For some (unknown to me) reason, however, the
code pretends that lock->old_sha1 has the new SHA-1 already before
we start to do the write or commit.
And because both write and commit tries to pretend to be no-op when
the caller tries to update a ref with the same SHA-1, but in this
codepath it does want the write to happen, it needs to set the
force_write bit set, which look like an unnecessary workaround.
Let me rephrase.
A natural way to write that caller, I think, would be more like
this:
... lock is given by the caller, probably with ->old_sha1
... filled in; it is not likely to be orig_sha1, as it is
... either null (if locking to create a new ref) or some
... unrelated value read from the ref being overwritten by
... this rename_ref() operation.
write_ref_sha1(lock, orig_sha1, logmsg);
# which may do the "don't write the same value" optmization
# if we are renaming another ref that happens to have the
# same value, in which case it is OK. Otherwise this will
# overwrite without being forced.
... *IF* and only if there is some reason lock->old_sha1
... needs to match what is in the filesystem ref right now,
... then do
hashcpy(lock->old_sha1, orig_sha1);
... but probably there is no reason to do so.
The two lines hashcpy() and ->force_write = 1 that appear before the
write_ref_sha1() do not conceptually make sense. The former does
not make sense because ->old_sha1 is supposed to be the original
value that is already stored in the ref, to allow us optimize "write
the same value" case, and you are breaking that invariant by doing
hashcpy() before write_ref_sha1(). The lock->old_sha1 value does
not have anything to do with the (original) value of the ref we took
the lock on.
And ->force_write = 1 becomes necessary only because the effect of
this nonsensical hashcpy() is to make the !hashcmp() used by the
short-cut logic trigger. Since the code needs to override that
logic, you need to say "force" before calling write_ref_sha1(). If
you didn't do the hashcpy(), you wouldn't have to say "force", 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] git-new-workdir: support submodules
Craig Silverstein writes: >>> Doesn't a submodule checkout keep some state tied to the working >>> tree in its repository configuration file? >> >> Do you mean, in 'config' itself? If so, I don't see it. (Though it's >> possible there are ways to use submodules that do keep working-tree >> state in the config file, and we just happen not to use those ways.) >> Here's what my webapp/.git/modules/khan-exercises/config looks like: >> --- >> [core] >> repositoryformatversion = 0 >> filemode = true >> bare = false >> logallrefupdates = true >> worktree = ../../../khan-exercises >> [remote "origin"] >> url = http://github.com/Khan/khan-exercises.git >> fetch = +refs/heads/*:refs/remotes/origin/* >> [branch "master"] >> remote = origin >> merge = refs/heads/master >> rebase = true >> [submodule "test/qunit"] >> url = https://github.com/jquery/qunit.git >> -- >> >> The only thing that seems vaguely working-tree related is the >> 'worktree' field, which is the field that motivated me to set up my >> patch the way it is. That is the location of the working tree of the top-level superproject. Tied to the state of the submodule working tree appear in [submodule "test/qunit"] part. In one new-workdir checkout, that submodule may be "submodule init"ed, while another one, it may not be. Or one new-workdir checkout's branch may check out a top-level project from today while the other one may have a top-level project from two years ago, and between these two checkouts of the top-level project, the settings of submodule."test/qunit".* variables may have to be different (perhaps even URL may have to point at two different repositories, one historical one to grab the state two years ago, the other current one). So sharing config between top-level checkouts may not be enough to "support submodules" (the patch title). -- To unsubscribe from this list: send the line "unsubscribe 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 20/24] update-index: test the system before enabling untracked cache
On Fri, Jan 23, 2015 at 1:49 AM, Junio C Hamano wrote: > I am not (yet) enthused by the intrusiveness of the overall series, though. I think the gain justifies the series' complexity. Although I don't mind redoing the whole series if we find a better way. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git submodule first time update with proxy
On Fri, Jan 23, 2015 at 4:13 PM, Chris Packham wrote: > Hi, > > On Fri, Jan 23, 2015 at 3:50 PM, Robert Dailey > wrote: >> I have a submodule using HTTP URL. I do this: >> >> $ git submodule init MySubmodule >> $ git submodule update MySubmodule >> >> The 2nd command fails because the HTTP URL cannot be resolved, this is >> because it requires a proxy. I have "http.proxy" setup properly in the >> .git/config of my parent git repository, so I was hoping the submodule >> update function would have a way to specify it to inherit the proxy >> value from the parent config. > > Your not the first to suggest it and you probably won't be the last. > It is hard to decide _which_ config variables, if any, should > propagate from the parent. What works for one use-case may not > necessarily work for another. > >> How can I set up my submodule? > > Probably the easiest thing would be to make your http.proxy > configuration global i.e. > > $ git config --global http.proxy > > If you don't want to make it a global setting you can setup the > submodule configuration after running init but before running update > i.e. > > $ git submodule init MySubmodule > $ (cd MySubmodule && git config http.proxy ...) > $ git submodule update MySubmodule For some reason, the init call does not create the submodule directory as you indicate. I also checked in .git/modules and it's not there either. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git submodule first time update with proxy
On Fri, Jan 23, 2015 at 10:23 PM, Robert Dailey wrote: > On Fri, Jan 23, 2015 at 4:13 PM, Chris Packham > wrote: >> Hi, >> >> On Fri, Jan 23, 2015 at 3:50 PM, Robert Dailey >> wrote: >>> I have a submodule using HTTP URL. I do this: >>> >>> $ git submodule init MySubmodule >>> $ git submodule update MySubmodule >>> >>> The 2nd command fails because the HTTP URL cannot be resolved, this is >>> because it requires a proxy. I have "http.proxy" setup properly in the >>> .git/config of my parent git repository, so I was hoping the submodule >>> update function would have a way to specify it to inherit the proxy >>> value from the parent config. >> >> Your not the first to suggest it and you probably won't be the last. >> It is hard to decide _which_ config variables, if any, should >> propagate from the parent. What works for one use-case may not >> necessarily work for another. >> >>> How can I set up my submodule? >> >> Probably the easiest thing would be to make your http.proxy >> configuration global i.e. >> >> $ git config --global http.proxy >> >> If you don't want to make it a global setting you can setup the >> submodule configuration after running init but before running update >> i.e. >> >> $ git submodule init MySubmodule >> $ (cd MySubmodule && git config http.proxy ...) >> $ git submodule update MySubmodule > > For some reason, the init call does not create the submodule > directory as you indicate. I also checked in .git/modules and it's not > there either. Correction: I have to deinit the submodule then init again, then the submodule dir is created (but empty). When I run the git config command inside the submodule directory, it silently returns and does not indicate failure, however the final git submodule update command shows failure to access the remote and then subsequently the submodule empty directory is removed by Git. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git pull not ignoring the file which has been sent to the temporary ignore list
On Friday, January 23, 2015 01:14:03 PM you wrote: > Stefan Beller writes: > > >> Ok. How should I then ignore any local changes to the .gitignore > >> file ? And while taking pull, git should skip this file ? > > > > Look at .git/info/exclude > > Good answer for ".gitignore". In general, you do not "ignore local > changes" to tracked paths. There are some configuration files, like `database.yml`, where we generally put our local DB credentials and we don't want to share such things. That's why we always put related settings inside the .gitignore file. But while I will change it, git will not track the changes of the file, but .gitignore. That's why I used the first thread command. But when the time the came to take a `git pull`, I got to know about the mess. What should be the ideal decision in this case ? > > I found https://help.github.com/articles/ignoring-files/ as Googles > > first hit, which advises to use > > git update-index --assume-unchanged path/to/file.txt > > Not sure if that is most helpful advice there. Yes, I followed the same. > The piece of advice in the last paragraph on that page is wrong (and > it has been wrong from the day it was written). > > The gitignore(5) documentation used to have a similar incorrect > piece of advice but we finally corrected it recently. > > Cf. http://thread.gmane.org/gmane.comp.version-control.git/260954/focus=261118 -- Regards, Arup Rakshit Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it. --Brian Kernighan -- To unsubscribe from this list: send the line "unsubscribe 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 submodule first time update with proxy
On Sat, Jan 24, 2015 at 5:45 PM, Robert Dailey wrote: > On Fri, Jan 23, 2015 at 10:23 PM, Robert Dailey > wrote: >> On Fri, Jan 23, 2015 at 4:13 PM, Chris Packham >> wrote: >>> Hi, >>> >>> On Fri, Jan 23, 2015 at 3:50 PM, Robert Dailey >>> wrote: I have a submodule using HTTP URL. I do this: $ git submodule init MySubmodule $ git submodule update MySubmodule The 2nd command fails because the HTTP URL cannot be resolved, this is because it requires a proxy. I have "http.proxy" setup properly in the .git/config of my parent git repository, so I was hoping the submodule update function would have a way to specify it to inherit the proxy value from the parent config. >>> >>> Your not the first to suggest it and you probably won't be the last. >>> It is hard to decide _which_ config variables, if any, should >>> propagate from the parent. What works for one use-case may not >>> necessarily work for another. >>> How can I set up my submodule? >>> >>> Probably the easiest thing would be to make your http.proxy >>> configuration global i.e. >>> >>> $ git config --global http.proxy >>> >>> If you don't want to make it a global setting you can setup the >>> submodule configuration after running init but before running update >>> i.e. >>> >>> $ git submodule init MySubmodule >>> $ (cd MySubmodule && git config http.proxy ...) >>> $ git submodule update MySubmodule >> >> For some reason, the init call does not create the submodule >> directory as you indicate. I also checked in .git/modules and it's not >> there either. > OK I must be wrong about that. I was working from memory. Trying it now I see the error in my thinking $ git submodule init bar Submodule 'bar' (bar.git) registered for path 'bar' I thought this meant that bar/.git (and .git/modules/bar) had been created but as you point out I was wrong. > Correction: > > I have to deinit the submodule then init again, then the submodule dir > is created (but empty). That's the default state of an uninitialized submodule. > When I run the git config command inside the > submodule directory, it silently returns and does not indicate > failure, however the final git submodule update command shows failure > to access the remote and then subsequently the submodule empty > directory is removed by Git. So it looks like the only solution to your problem right now is to use git config --global for your proxy configuration. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
