BUG: PATHSPEC_PREFER_CWD requires arguments
Hello,
I ran into the following bug today: BUG: PATHSPEC_PREFER_CWD requires
arguments. It's not that bad because I'm trying to run `git log
--merge` on an already resolved conflict. Still, I don't think I
should hit a BUG: :-)
Here is a script to reproduce:
git init .
a
git add a
git commit -mcreate a
git branch other
echo 1 a
git commit -madd 1 a
git checkout other
echo 2 a
git commit -madd 2 a
git merge master
git add a
git log --merge -- a
# Fails with fatal: BUG: PATHSPEC_PREFER_CWD requires arguments
Here is what GDB gives me when I break on die():
Breakpoint 1, die (err=0x57e3a8 BUG: PATHSPEC_PREFER_CWD requires
arguments) at usage.c:97
97 if (die_is_recursing()) {
(gdb) bt
#0 die (err=0x57e3a8 BUG: PATHSPEC_PREFER_CWD requires arguments)
at usage.c:97
#1 0x004ea58a in parse_pathspec (pathspec=0x7fffc288,
magic_mask=31, flags=0, prefix=0x580dad , argv=0x0) at
pathspec.c:377
#2 0x005097b4 in prepare_show_merge (revs=0x7fffc240) at
revision.c:1375
#3 0x0050c32e in setup_revisions (argc=2,
argv=0x7fffcb08, revs=0x7fffc240, opt=0x7fffc220) at
revision.c:2147
#4 0x00446efc in cmd_log_init_finish (argc=4,
argv=0x7fffcb08, prefix=0x0, rev=0x7fffc240,
opt=0x7fffc220)
at builtin/log.c:147
#5 0x0044716a in cmd_log_init (argc=4, argv=0x7fffcb08,
prefix=0x0, rev=0x7fffc240, opt=0x7fffc220) at
builtin/log.c:203
#6 0x00448349 in cmd_log (argc=4, argv=0x7fffcb08,
prefix=0x0) at builtin/log.c:635
#7 0x0040584a in run_builtin (p=0x7bdb30, argc=4,
argv=0x7fffcb08) at git.c:314
#8 0x004059d5 in handle_internal_command (argc=4,
argv=0x7fffcb08) at git.c:478
#9 0x00405b88 in main (argc=4, av=0x7fffcb08) at git.c:575
And here is what bisect found:
commit 9a0872744315da67db3c81eb9270751e31fcc8f5
Author: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Date: Sun Jul 14 15:35:59 2013 +0700
remove init_pathspec() in favor of parse_pathspec()
While at there, move free_pathspec() to pathspec.c
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
Thanks,
Antoine
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/2] checkout: allow dwim for branch creation for git checkout $branch --
The -- notation disambiguates files and branches, but as a side-effect
of the previous implementation, also disabled the branch auto-creation
when $branch does not exist.
A possible scenario is then:
git checkout $branch
= fails if $branch is both a ref and a file, and suggests --
git checkout $branch --
= refuses to create the $branch
This patch allows the second form to create $branch, and since the -- is
provided, it does not look for file named $branch.
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
So it may be even cleaner to read if you did it this way:
Indeed. The code reads better now.
builtin/checkout.c | 78 ++--
t/t2024-checkout-dwim.sh | 21 +
2 files changed, 76 insertions(+), 23 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0f57397..2003795 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -885,20 +885,30 @@ static int parse_branchname_arg(int argc, const char
**argv,
*
* everything after the '--' must be paths.
*
-* case 3: git checkout something [paths]
+* case 3: git checkout something [--]
*
-* With no paths, if something is a commit, that is to
-* switch to the branch or detach HEAD at it. As a special case,
-* if something is A...B (missing A or B means HEAD but you can
-* omit at most one side), and if there is a unique merge base
-* between A and B, A...B names that merge base.
+* (a) If something is a commit, that is to
+* switch to the branch or detach HEAD at it. As a special case,
+* if something is A...B (missing A or B means HEAD but you can
+* omit at most one side), and if there is a unique merge base
+* between A and B, A...B names that merge base.
*
-* With no paths, if something is _not_ a commit, no -t nor -b
-* was given, and there is a tracking branch whose name is
-* something in one and only one remote, then this is a short-hand
-* to fork local something from that remote-tracking branch.
+* (b) If something is _not_ a commit, either -- is present
+* or something is not a path, no -t nor -b was given, and
+* and there is a tracking branch whose name is something
+* in one and only one remote, then this is a short-hand to
+* fork local something from that remote-tracking branch.
*
-* Otherwise something shall not be ambiguous.
+* (c) Otherwise, if -- is present, treat it like case (1).
+*
+* (d) Otherwise :
+* - if it's a reference, treat it like case (1)
+* - else if it's a path, treat it like case (2)
+* - else: fail.
+*
+* case 4: git checkout something paths
+*
+* The first argument must not be ambiguous.
* - If it's *only* a reference, treat it like case (1).
* - If it's only a path, treat it like case (2).
* - else: fail.
@@ -917,18 +927,40 @@ static int parse_branchname_arg(int argc, const char
**argv,
arg = @{-1};
if (get_sha1_mb(arg, rev)) {
- if (has_dash_dash) /* case (1) */
- die(_(invalid reference: %s), arg);
- if (dwim_new_local_branch_ok
- !check_filename(NULL, arg)
- argc == 1) {
+ /*
+* Either case (3) or (4), with something not being
+* a commit, or an attempt to use case (1) with an
+* invalid ref.
+*
+* It's likely an error, but we need to find out if
+* we should auto-create the branch, case (3).(b).
+*/
+ int recover_with_dwim = dwim_new_local_branch_ok;
+
+ if (check_filename(NULL, arg) !has_dash_dash)
+ recover_with_dwim = 0;
+ /*
+* Accept git checkout foo and git checkout foo --
+* as candidates for dwim.
+*/
+ if (!(argc == 1 !has_dash_dash)
+ !(argc == 2 has_dash_dash))
+ recover_with_dwim = 0;
+
+ if (recover_with_dwim) {
const char *remote = unique_tracking_name(arg, rev);
- if (!remote)
- return argcount;
- *new_branch = arg;
- arg = remote;
- /* DWIMmed to create local branch */
- } else {
+ if (remote) {
+ *new_branch = arg;
+ arg = remote;
+ /* DWIMmed to create local branch, case (3).(b)
*/
+
[PATCH v4 2/2] checkout: proper error message on 'git checkout foo bar --'
The previous code was detecting the presence of -- by looking only at
argument 1. As a result, git checkout foo bar -- was interpreted as an
ambiguous file/revision list, and errored out with:
error: pathspec 'foo' did not match any file(s) known to git.
error: pathspec 'bar' did not match any file(s) known to git.
error: pathspec '--' did not match any file(s) known to git.
This patch fixes it by walking through the argument list to find the
--, and now complains about the number of references given.
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
Unchanged since v3.
builtin/checkout.c| 21 -
t/t2010-checkout-ambiguous.sh | 6 ++
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2003795..54f80bd 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -873,7 +873,9 @@ static int parse_branchname_arg(int argc, const char **argv,
int argcount = 0;
unsigned char branch_rev[20];
const char *arg;
- int has_dash_dash;
+ int dash_dash_pos;
+ int has_dash_dash = 0;
+ int i;
/*
* case 1: git checkout ref -- [paths]
@@ -917,11 +919,20 @@ static int parse_branchname_arg(int argc, const char
**argv,
if (!argc)
return 0;
- if (!strcmp(argv[0], --)) /* case (2) */
- return 1;
-
arg = argv[0];
- has_dash_dash = (argc 1) !strcmp(argv[1], --);
+ dash_dash_pos = -1;
+ for (i = 0; i argc; i++) {
+ if (!strcmp(argv[i], --)) {
+ dash_dash_pos = i;
+ break;
+ }
+ }
+ if (dash_dash_pos == 0)
+ return 1; /* case (2) */
+ else if (dash_dash_pos == 1)
+ has_dash_dash = 1; /* case (3) or (1) */
+ else if (dash_dash_pos = 2)
+ die(_(only one reference expected, %d given.), dash_dash_pos);
if (!strcmp(arg, -))
arg = @{-1};
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
index 7cc0a35..87bdf9c 100755
--- a/t/t2010-checkout-ambiguous.sh
+++ b/t/t2010-checkout-ambiguous.sh
@@ -47,4 +47,10 @@ test_expect_success 'disambiguate checking out from a
tree-ish' '
git diff --exit-code --quiet
'
+test_expect_success 'accurate error message with more than one ref' '
+ test_must_fail git checkout HEAD master -- 2actual
+ grep 2 actual
+ test_i18ngrep one reference expected, 2 given actual
+'
+
test_done
--
1.8.4.479.g0ed768e
--
To unsubscribe from this list: send the line 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 v8] diff.c: keep arrow(=) on show_stats()'s shortened filename part to make rename visible
git diff -M --stat can detect rename and show renamed file name like
foofoofoo = barbarbar.
Before this commit, this output is shortened always by omitting left most
part like ...foo = barbarbar. So, if the destination filename is too long,
source filename putting left or arrow can be totally omitted like
...barbarbar, without including any of foofoofoo =.
In such a case where arrow symbol is omitted, there is no way to know
whether the file is renamed or existed in the original.
Make sure there is always an arrow, like ...foo = ...bar.
The output can contain curly braces('{','}') for grouping.
So, in general, the output format is pfx{mid_a = mid_b}sfx
To keep arrow(=), try to omit pfx as long as possible at first
because later part or changing part will be the more important part.
If it is not enough, shorten mid_a, mid_b trying to have the same
maximum length.
If it is not enough yet, omit sfx.
Signed-off-by: Tsuneo Yoshioka yoshiokatsu...@gmail.com
Test-added-by: Thomas Rast tr...@inf.ethz.ch
---
diff.c | 176 ++---
t/t4001-diff-rename.sh | 12
2 files changed, 166 insertions(+), 22 deletions(-)
diff --git a/diff.c b/diff.c
index a04a34d..69c3e17 100644
--- a/diff.c
+++ b/diff.c
@@ -1258,11 +1258,13 @@ static void fn_out_consume(void *priv, char *line,
unsigned long len)
}
}
-static char *pprint_rename(const char *a, const char *b)
+static void find_common_prefix_suffix(const char *a, const char *b,
+ struct strbuf *pfx,
+ struct strbuf *a_mid, struct strbuf *b_mid,
+ struct strbuf *sfx)
{
const char *old = a;
const char *new = b;
- struct strbuf name = STRBUF_INIT;
int pfx_length, sfx_length;
int pfx_adjust_for_slash;
int len_a = strlen(a);
@@ -1272,10 +1274,9 @@ static char *pprint_rename(const char *a, const char *b)
int qlen_b = quote_c_style(b, NULL, NULL, 0);
if (qlen_a || qlen_b) {
- quote_c_style(a, name, NULL, 0);
- strbuf_addstr(name, = );
- quote_c_style(b, name, NULL, 0);
- return strbuf_detach(name, NULL);
+ quote_c_style(a, a_mid, NULL, 0);
+ quote_c_style(b, b_mid, NULL, 0);
+ return;
}
/* Find common prefix */
@@ -1322,17 +1323,138 @@ static char *pprint_rename(const char *a, const char
*b)
if (b_midlen 0)
b_midlen = 0;
- strbuf_grow(name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
- if (pfx_length + sfx_length) {
- strbuf_add(name, a, pfx_length);
+ strbuf_add(pfx, a, pfx_length);
+ strbuf_add(a_mid, a + pfx_length, a_midlen);
+ strbuf_add(b_mid, b + pfx_length, b_midlen);
+ strbuf_add(sfx, a + len_a - sfx_length, sfx_length);
+}
+
+/*
+ * Omit each parts to fix in name_width.
+ * Formatted string is pfx{a_mid = b_mid}sfx.
+ * At first, omit pfx as long as possible.
+ * If it is not enough, omit a_mid, b_mid trying to set the length of
+ * those 2 parts(including ...) to the same.
+ * If it is not enough yet, omit sfx.
+ * Ex:
+ * foofoofoo = barbarbar
+ * will be like
+ * ...foo = ...bar.
+ * long_parent{foofoofoo = barbarbar}path/filename
+ * will be like
+ * ...parent{...foofoo = ...barbar}path/filename
+ */
+static void rename_omit(struct strbuf *pfx,
+ struct strbuf *a_mid, struct strbuf *b_mid,
+ struct strbuf *sfx,
+ int name_width)
+{
+ static const char arrow[] = = ;
+ static const char dots[] = ...;
+ int use_curly_braces = (pfx-len 0) || (sfx-len 0);
+ size_t name_len;
+ size_t max_part_len = 0;
+ size_t remainder_part_len = 0;
+ size_t left, right;
+ size_t max_sfx_len;
+ size_t sfx_len;
+
+ name_len = pfx-len + a_mid-len + b_mid-len + sfx-len + strlen(arrow)
+ + (use_curly_braces ? 2 : 0);
+
+ if (name_len = name_width)
+ return; /* everything fits in name_width */
+
+ if (use_curly_braces) {
+ if (strlen(dots) + (name_len - pfx-len) = name_width) {
+ /*
+* Just omitting left of '{' is enough
+* Ex: ...aaa{foofoofoo = bar}file
+*/
+ strbuf_splice(pfx, 0, name_len - name_width +
strlen(dots), dots, strlen(dots));
+ return;
+ } else {
+ if (pfx-len strlen(dots)) {
+ /*
+* Just omitting left of '{' is not enough
+* name will be ...{SOMETHING}SOMETHING
+*/
+ strbuf_reset(pfx);
+
Re: [PATCH v6] diff.c: keep arrow(=) on show_stats()'s shortened filename part to make rename visible.
Hello Junio
In the [PATCH v7], I changed to keep filename part of suffix to handle
above case, but not always keep directory part because I feel totally
keeping all part of long suffix including directory name may cause output
like:
…{… = …}…ongPath1/LongPath2/nameOfTheFileThatWasMoved
And, above may be worse than:
...{...ceDirectory = …ionDirectory}.../nameOfTheFileThatWasMoved
I think.
I am not sure if I agree.
Losing LongPath2 part may be more significant data loss than losing
a single bit that says the change is a rename, as the latter may not
quite tell us what these two directories were anyway.
I'm not sure which is the better in general.
But anyway, I don't have strong opinion about this.
So, I just changed to keep the all of the sfx part(lator than '}').
I just sent the updated patch as [PATCH v8].
Thanks !
---
Tsuneo Yoshioka (吉岡 恒夫)
yoshiokatsu...@gmail.com
On Oct 18, 2013, at 1:38 AM, Junio C Hamano gits...@pobox.com wrote:
Yoshioka Tsuneo yoshiokatsu...@gmail.com writes:
In the [PATCH v7], I changed to keep filename part of suffix to handle
above case, but not always keep directory part because I feel totally
keeping all part of long suffix including directory name may cause output
like:
…{… = …}…ongPath1/LongPath2/nameOfTheFileThatWasMoved
And, above may be worse than:
...{...ceDirectory = …ionDirectory}.../nameOfTheFileThatWasMoved
I think.
I am not sure if I agree.
Losing LongPath2 part may be more significant data loss than losing
a single bit that says the change is a rename, as the latter may not
quite tell us what these two directories were anyway.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] Officially start moving to the term 'staging area'
I'm lacking time to read and answer in detail, sorry. Junio C Hamano gits...@pobox.com writes: It must be done is different from any change is good, as long as it introduces more instances of word 'stage'. I agree. Something must be done, at least to remove the cache Vs index confusion. I'm not sure exactly what's best, and we should agree where to go before going there. The previous attempts to introduce more stage in Git's command-line (e.g. the git stage alias) introduced more confusion than anything else. The phrase staging area is not an everyday phrase or common CS lingo, and that unfortunately makes it a suboptimal choice of words especially to those of us, to whom a large portion of their exposure to the English language is through the command words we use when we talk to our computers. I do not think being understandable immediately by non-native is so important actually. To me as a french, commit makes no sense as an english word to describe what git commit does, but it's OK as I never really translate it. Even fr.po translates a commit by un commit. That said, having something that immediately makes sense to a non-native is obviously a good point. Another proposal which I liked BTW was to use the word precommit. Short, and easily understood as the place where the next commit is prepared. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] Officially start moving to the term 'staging area'
On Fri, Oct 18, 2013 at 5:46 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: I'm lacking time to read and answer in detail, sorry. Junio C Hamano gits...@pobox.com writes: It must be done is different from any change is good, as long as it introduces more instances of word 'stage'. I agree. Something must be done, at least to remove the cache Vs index confusion. I'm not sure exactly what's best, and we should agree where to go before going there. The previous attempts to introduce more stage in Git's command-line (e.g. the git stage alias) introduced more confusion than anything else. I definitely agree about removing the cache vs. index confusion. I'm curious about the confusions surrounding this git stage alias. Was it simply an implementation issue, or was it an issue surrounding the name? FWIW, I've trained my employees to think of it as a staging area as well. At least in English, it seems to be the best understood analogy to the index's purpose. The phrase staging area is not an everyday phrase or common CS lingo, and that unfortunately makes it a suboptimal choice of words especially to those of us, to whom a large portion of their exposure to the English language is through the command words we use when we talk to our computers. I do not think being understandable immediately by non-native is so important actually. To me as a french, commit makes no sense as an english word to describe what git commit does, but it's OK as I never really translate it. Even fr.po translates a commit by un commit. That said, having something that immediately makes sense to a non-native is obviously a good point. Another proposal which I liked BTW was to use the word precommit. Short, and easily understood as the place where the next commit is prepared. I'm not sure what concept precommit invokes, but it's certainly not where the next commit is prepared. Two thoughts come to mind: the precommit hook, and what is a pre-commit? How would you talk about preparing for a commit? Do you precommit a file? Add the file to the precommit? I'm just curious. Thanks! -John -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] Officially start moving to the term 'staging area'
On Fri, Oct 18, 2013 at 4:46 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: I'm lacking time to read and answer in detail, sorry. Junio C Hamano gits...@pobox.com writes: It must be done is different from any change is good, as long as it introduces more instances of word 'stage'. I agree. Something must be done, at least to remove the cache Vs index confusion. I'm not sure exactly what's best, and we should agree where to go before going there. I thought we already agreed staging area is the best term. Some people don't, but that's expected. The phrase staging area is not an everyday phrase or common CS lingo, and that unfortunately makes it a suboptimal choice of words especially to those of us, to whom a large portion of their exposure to the English language is through the command words we use when we talk to our computers. I do not think being understandable immediately by non-native is so important actually. To me as a french, commit makes no sense as an english word to describe what git commit does, but it's OK as I never really translate it. Even fr.po translates a commit by un commit. Indeed. Let's hope this red herring is not brought again. That said, having something that immediately makes sense to a non-native is obviously a good point. Most non-native speakers, as most native speakers, already agreed the term staging area is best. Another proposal which I liked BTW was to use the word precommit. Short, and easily understood as the place where the next commit is prepared. And that proposal has been argued against already[1][2]. To summarize: 1) It's not even an English word 2) Unlike staging area, it's not widely used in external documentation already 3) There's no sensible verb: to precommit? Moreover, in my mind a true precommit would have author, committer, date; all the things you expect in a commit, except that it's not permanent. A natural command that would derive from this concept is 'git commit --prepare', which would create an actual precommit. But we are not looking to introduce yet another concept, we are looking for a name of a concept we already have, and the majority of users have already given it a name; the staging area. [1] http://article.gmane.org/gmane.comp.version-control.git/197215 [2] http://article.gmane.org/gmane.comp.version-control.git/168201 -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] Officially start moving to the term 'staging area'
On 17.10.2013, at 21:50, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: [...] However, since you asked, I would share a couple of comments regarding the index, cache and staging area. (1) Staging area. The phrase staging area is not an everyday phrase or common CS lingo, and that unfortunately makes it a suboptimal choice of words especially to those of us, to whom a large portion of their exposure to the English language is through the command words we use when we talk to our computers. Interestingly, as a non-native speaker, I draw the exact reverse conclusion from this: While I had no idea what a staging area or to stage was (I did know the stage in a theater, though), I found this to not be a major problem: Using a dictionary and reading up on what it means in git made it clearly quickly enough. To the contrary, the fact that the term was not yet overloaded with conflicting other meanings made it easier for me to attach the semantics git associates with it. In contrast, index was exceedingly bad and confusing to me... I already had various notions of what an index is (e.g. the index of a book -- the same word actually exists in German; or more generally an index in computer science, as a kind of loopup table, etc.), and to this day, have a hard time consolidating this with the way git uses it. For me, it is yet another, seeming completely unrelated, meaning of the word index I had to memorize. Hey, just take a look at Wiki page http://en.wikipedia.org/wiki/Index for the many dozens of meanings associated to this word. Ugh. And worst of it, I am actually not quite sure on which of the meanings listed there the index as used by git is based... I.e. I don't even see a helpful analogy that would make it easier to understand the choice of name. In summary: For me as a non-native speaker, index feels like about the worst possible choice (well, you could have called it the file or thing, that might have been worse ;-). While staging area turned out to be surprisingly good, *precisely* because I was unfamiliar with it. So, while staging area might not be perfect, it seems good enough to me. If this matter had indeed been discussed here for years, and no better suggestions has come up, then perhaps it is time to end the search for the (possibly non-existent) perfect solution, and instead do the pragmatic? The index can also be thought of like the buffer cache, where new contents of files are kept before they are committed to disk platter. At least, non-native speaker with CS background would understand that, much better than the index (no, I am not saying that we should call it the cache; I am just saying the index is not a good word, but we may need to find a better word than the staging area). Huh? As a non-native speaker with CS background, this actually leaves me more confused than I was before. I think about the staging area, and I don't see how this is anything like an index (in any of the meaning I see on http://en.wikipedia.org/wiki/Index). I can vaguely recognize a faint similarity to a cache, and yet more relation to a buffer, but in the end, none of these strike me as particularly illustrative. For that matter, I never really understood of why I had to do git diff --cached, I simply learned it by rote. On the other hand, I feel that after understanding what the staging area is, then writing git diff --staged is very logical and simple to memorize. Cheers, Max signature.asc Description: Message signed with OpenPGP using GPGMail
Re: My patches
I guess most other people keep out of this because they are sensible enough to not feed the troll, and instead focus on useful things. But I can't help it, I have to say this. On 17.10.2013, at 23:44, Felipe Contreras felipe.contre...@gmail.com wrote: Junio C Hamano wrote: Felipe Contreras felipe.contre...@gmail.com writes: Junio C Hamano wrote: Such a review comment and the discussion that follows it after a patch is posted is an essential part of the collaborative development process in this community and it has helped the quality of our end product. We unfortunately saw time and again that the process rarely works when the discussion involves your patches. No, you did not. What you saw was a person that unlike a trained dog, argued against you. And apparently your definition of a good discussion is one in which the other person just does what you say, and doesn't argue back. That is so untrue that it is not even funny. It is true, and there's penty of evidence that proves it. No, it is not true, and there is plenty of evidence that proves it. But I don't think it's helpful for either of us drag up such evidence, as it'll convince nobody -- indeed, I am sure almost everybody here has already formed a clear opinion on this matter. And I hazard to guess that the vast majority agree with Junio on this (based, again, on email evidence). Not with you. Of course one can't prove mathematical theorems by a majority vote, but as we are not talking about theorems, but rather about judging whether Junion's behavior is considered fair or not, I think it is appropriate to. Moreover, if I look at e.g. the staging area discussion, you also bring up the everybody but Junio and one other guy argument (incorrectly generalizing from those people on this mailing list who chose to reply to everybody), so I think I am entitled to do the same ;-). (BTW, I am actually in favor of using the term staging area over index) Every single time that you get mad at me, it's because I disagree with you. I have not yet seen Junio get mad here, even in discussions with you were I think most other people would indeed have gotten mad at you. He stays remarkably polite, despite the insults and dirt you keep flinging at him... If at all, it would seem that you are getting mad at Junio. Contributors often make sensible counter-arguments and/or explain the rationale better than they did in their original messages, and then receive a Thanks for a dose of sanity (or an equivalent phrased in different ways). Yes, when there's an agreement, so you are basically proving what I said. I disagree with you, you disagree with me, and that means I'm the problem. Actually, it is more like this: Felipe disagrees with Junio, Junio disagrees with Felipe, Felipe insults Junio and in passing half a dozen other people. It is the latter point which makes the situation asymmetric, and indeed indicates you as the problem. In any healthy collaborative project that simply means there was a disagreement, and that's that. If your premise was correct (that there is simply a disagreement), then this conclusion might be correct. As it is, though, your premise is false. The problem is rather a disruptive person: you. Quite sad, since you seem to have some good ideas and code contributions! I am in particular grateful for your work on remote helpers, both on specific ones (git-remote-hg) and also on improving the whole remote helper interface. I hope some of this work can eventually be merged... But at the end of the day, we most (all?) of us here are volunteers, and unlike what you seem to claim a lot, for most of us, making git better is *not* the number 1 priority in our lives... In particular, if working with you would make git better, but at the same time would give me ulcers, well, my choice is clear to me... Perhaps it wouldn't be best for git, but I don't think anybody (except, perhaps, you) would blame me for it. Or, for that matter, Junio. @Junio: Thank you for being an opinionated but also very fair project lead, who listens to *constructive* feedback, and has again and again demonstrated through action a readiness to revise a position based on sensible discussions conducted on this list. Max signature.asc Description: Message signed with OpenPGP using GPGMail
Re: My patches
Max Horn wrote: I guess most other people keep out of this because they are sensible enough to not feed the troll, and instead focus on useful things. But I can't help it, I have to say this. You should probably read the definition of troll: https://en.wikipedia.org/wiki/Troll_(Internet) Unless you think that contributing useful patches to Git is off-topic, a person that does that by definition cannot be a troll. On 17.10.2013, at 23:44, Felipe Contreras felipe.contre...@gmail.com wrote: Junio C Hamano wrote: Felipe Contreras felipe.contre...@gmail.com writes: Junio C Hamano wrote: Such a review comment and the discussion that follows it after a patch is posted is an essential part of the collaborative development process in this community and it has helped the quality of our end product. We unfortunately saw time and again that the process rarely works when the discussion involves your patches. No, you did not. What you saw was a person that unlike a trained dog, argued against you. And apparently your definition of a good discussion is one in which the other person just does what you say, and doesn't argue back. That is so untrue that it is not even funny. It is true, and there's penty of evidence that proves it. No, it is not true, and there is plenty of evidence that proves it. But I don't think it's helpful for either of us drag up such evidence, as it'll convince nobody -- indeed, I am sure almost everybody here has already formed a clear opinion on this matter. That's why I didn't bring it up. And I hazard to guess that the vast majority agree with Junio on this (based, again, on email evidence). Not with you. That is irrelevant, and a fallacy. The vast majority of people thought the Earth was the center of the universe, and they were all wrong. It's called ad populum fallacy, look it up. Wether the majority of Git developers agree that there's something more than a disagreement is irrelevant, their opinion doesn't change the truth. And by the way, a disregard for evidence is a clear sign of a person that is not interested in the truth. Of course one can't prove mathematical theorems by a majority vote, but as we are not talking about theorems, but rather about judging whether Junion's behavior is considered fair or not, I think it is appropriate to. No, that's not what we are talking about. My claim is that all I did was disagree with Junio. You can invalidate that claim easily by providing *a single* example where I did more than disagree. Moreover, if I look at e.g. the staging area discussion, you also bring up the everybody but Junio and one other guy argument (incorrectly generalizing from those people on this mailing list who chose to reply to everybody), so I think I am entitled to do the same ;-). I've stated multiple times that by everybody I mean virtually everybody. Since Junio has accepted that the index is not the best term, virtually everybody is actually everybody that has voiced an opinion, except one single person. Every single time that you get mad at me, it's because I disagree with you. I have not yet seen Junio get mad here, even in discussions with you were I think most other people would indeed have gotten mad at you. I can provide the evidence when Junio has become clearly mad... If you are interested in the truth that is. He stays remarkably polite, despite the insults and dirt you keep flinging at him... If at all, it would seem that you are getting mad at Junio. That is pure libel. Show me *one* example where I threw an insult, or dirt. Contributors often make sensible counter-arguments and/or explain the rationale better than they did in their original messages, and then receive a Thanks for a dose of sanity (or an equivalent phrased in different ways). Yes, when there's an agreement, so you are basically proving what I said. I disagree with you, you disagree with me, and that means I'm the problem. Actually, it is more like this: Felipe disagrees with Junio, Junio disagrees with Felipe, Felipe insults Junio and in passing half a dozen other people. Lies. When did I insult anybody? In any healthy collaborative project that simply means there was a disagreement, and that's that. If your premise was correct (that there is simply a disagreement), then this conclusion might be correct. As it is, though, your premise is false. Evidence, or that claim is dismissed. That which can be asserted without evidence can be dismissed without evidence. The problem is rather a disruptive person: you. Nelson Mandela was considered a disruptive person (a terrorist[1]), yet virtually everyone agrees that the problem was not him, but the system that labeled him as such. [1] http://en.wikinews.org/wiki/Nelson_Mandela_taken_off_US_terrorist_list -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to
[PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
There are cases (e.g. when running concurrent fetches in a repo) where multiple Git processes concurrently attempt to create loose objects within the same objects/XX/ dir. The creation of the loose object files is (AFAICS) safe from races, but the creation of the objects/XX/ dir in which the loose objects reside is unsafe, for example: Two concurrent fetches - A and B. As part of its fetch, A needs to store 12a as a loose object. B, on the other hand, needs to store 12b as a loose object. The objects/12 directory does not already exist. Concurrently, both A and B determine that they need to create the objects/12 directory (because their first call to git_mkstemp_mode() within create_tmpfile() fails witn ENOENT). One of them - let's say A - executes the following mkdir() call before the other. This first call returns success, and A moves on. When B gets around to calling mkdir(), it fails with EEXIST, because A won the race. The mkdir() error causes B to return -1 from create_tmpfile(), which propagates all the way, resulting in the fetch failing with: error: unable to create temporary file: File exists fatal: failed to write object fatal: unpack-objects failed Although it's hard to add a testcase reproducing this issue, it's easy to reproduce if we insert a sleep after the if (mkdir(buffer, 0777) || adjust_shared_perm(buffer)) return -1; block, and then run two concurrent git fetches against the same repo. The fix is to simply handle mkdir() setting errno = EEXIST as success. Signed-off-by: Johan Herland jo...@herland.net --- sha1_file.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sha1_file.c b/sha1_file.c index f80bbe4..00e 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2857,7 +2857,9 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) /* Make sure the directory exists */ memcpy(buffer, filename, dirlen); buffer[dirlen-1] = 0; - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer)) + if (mkdir(buffer, 0777) errno != EEXIST) + return -1; + if (adjust_shared_perm(buffer)) return -1; /* Try again */ -- 1.8.4.653.g2df02b3 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: cherry-pick generates bogus conflicts on removed files
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 10/17/2013 5:14 PM, Junio C Hamano wrote: Correct. Without inspecting them, you would not know what you would be losing by blindly resolving to removal, hence we do not auto-resolve one side removed, the other side changed to a removal. Even when I specify the theirs strategy? It seems like saying to unconditionally accept their changes should not generate conflicts. That does not need to mean that we should not make it easier for the user to say resolve these 'one side removed, the other side changed' paths to removal. add -u will be a way to say Record the changes I made to my working tree files to the index. So presumably rm -f those files that the other branch removed git add -u would be one way to do so. Of course, you can also use git rm directly, i.e. git rm -f those files that the other branch removed I have about two dozen such files. Are you saying I have to individually remove each one? -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.17 (MingW32) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJSYTbNAAoJEJrBOlT6nu752lAH/33YymYr773UZY8ryioA7dkQ he3qjYzxWdIY2FtNFfYER1o1Q7PpeDLvq61b67IZ2htFjiK4+xgM/q+wjp3RMkXI sA2vQ9iNn8dvR+PlzR9DgOFcDD17p3Q/xbu3H/M4Nt+H3px+Yz6sjUUSOzDAzXl8 ADQe0g4KkQnY4fjx+iWbrygY5xXCaQ52693pwYvR67GijfYxIuNb4d9DpkZhyqIZ L6qjJH4FR1AZl6n5hPj0a9ZitrO8J/MjJ24oLVBfBU2h1GF5h7LoavkU02S874BZ /8fULrCYfTCMSkyOCnk2xCkhw3sbqU9vEu90npI5X1nRbZZ0ro/DPAjXCgHB40c= =EB2w -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe 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] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland jo...@herland.net wrote: There are cases (e.g. when running concurrent fetches in a repo) where multiple Git processes concurrently attempt to create loose objects within the same objects/XX/ dir. The creation of the loose object files is (AFAICS) safe from races, but the creation of the objects/XX/ dir in which the loose objects reside is unsafe, for example: Two concurrent fetches - A and B. As part of its fetch, A needs to store 12a as a loose object. B, on the other hand, needs to store 12b as a loose object. The objects/12 directory does not already exist. Concurrently, both A and B determine that they need to create the objects/12 directory (because their first call to git_mkstemp_mode() within create_tmpfile() fails witn ENOENT). One of them - let's say A - executes the following mkdir() call before the other. This first call returns success, and A moves on. When B gets around to calling mkdir(), it fails with EEXIST, because A won the race. The mkdir() error causes B to return -1 from create_tmpfile(), which propagates all the way, resulting in the fetch failing with: error: unable to create temporary file: File exists fatal: failed to write object fatal: unpack-objects failed Although it's hard to add a testcase reproducing this issue, it's easy to reproduce if we insert a sleep after the if (mkdir(buffer, 0777) || adjust_shared_perm(buffer)) return -1; block, and then run two concurrent git fetches against the same repo. The fix is to simply handle mkdir() setting errno = EEXIST as success. Would it make sense for the commit message to explain what happens if EEXIST is returned but the pathname is not a directory? (For instance, in the same race window, another process had created a plain file or pipe there.) Signed-off-by: Johan Herland jo...@herland.net --- sha1_file.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sha1_file.c b/sha1_file.c index f80bbe4..00e 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2857,7 +2857,9 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) /* Make sure the directory exists */ memcpy(buffer, filename, dirlen); buffer[dirlen-1] = 0; - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer)) + if (mkdir(buffer, 0777) errno != EEXIST) + return -1; + if (adjust_shared_perm(buffer)) return -1; /* Try again */ -- 1.8.4.653.g2df02b3 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Fri, Oct 18, 2013 at 8:17 PM, Johan Herland jo...@herland.net wrote: diff --git a/sha1_file.c b/sha1_file.c index f80bbe4..00e 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2857,7 +2857,9 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) /* Make sure the directory exists */ memcpy(buffer, filename, dirlen); buffer[dirlen-1] = 0; - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer)) + if (mkdir(buffer, 0777) errno != EEXIST) + return -1; + if (adjust_shared_perm(buffer)) return -1; I was going to ask what if the created directory does not have permission 0777. But adjust_shared_perm follows immediately after, so we're good. It also answers the question why mkdir(buffer, mode) errno != EEXIST should not be wrapped in xmkdir() or something to be used in other places as well. Thanks. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Fri, Oct 18, 2013 at 8:55 PM, Duy Nguyen pclo...@gmail.com wrote: On Fri, Oct 18, 2013 at 8:17 PM, Johan Herland jo...@herland.net wrote: diff --git a/sha1_file.c b/sha1_file.c index f80bbe4..00e 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2857,7 +2857,9 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) /* Make sure the directory exists */ memcpy(buffer, filename, dirlen); buffer[dirlen-1] = 0; - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer)) + if (mkdir(buffer, 0777) errno != EEXIST) + return -1; + if (adjust_shared_perm(buffer)) return -1; I was going to ask what if the created directory does not have permission 0777. But adjust_shared_perm follows immediately after, so we're good. And I sent too early. adjust_shared_perm() does rely on current dir's permission. So if the creator does mkdir(buffer, 0), we still have a bad permission in the end. And adjust_shared_perm() is only active when the repository is configured to be shared. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Fri, Oct 18, 2013 at 3:53 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland jo...@herland.net wrote: There are cases (e.g. when running concurrent fetches in a repo) where multiple Git processes concurrently attempt to create loose objects within the same objects/XX/ dir. The creation of the loose object files is (AFAICS) safe from races, but the creation of the objects/XX/ dir in which the loose objects reside is unsafe, for example: [...] The fix is to simply handle mkdir() setting errno = EEXIST as success. Would it make sense for the commit message to explain what happens if EEXIST is returned but the pathname is not a directory? (For instance, in the same race window, another process had created a plain file or pipe there.) I'm pretty sure in that case, the following adjust_shared_perm() or git_mkstemp_mode() would fail, and we'd end up returning error from create_tmpfile() in any case. I can add the above to the commit message if you insist. :) ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: My patches
On Fri, Oct 18, 2013 at 06:41:41AM -0500, Felipe Contreras wrote: And I hazard to guess that the vast majority agree with Junio on this (based, again, on email evidence). Not with you. That is irrelevant, and a fallacy. The vast majority of people thought the Earth was the center of the universe, and they were all wrong. It's called ad populum fallacy, look it up. Wether the majority of Git developers agree that there's something more than a disagreement is irrelevant, their opinion doesn't change the truth. Look, the problem is that you insist on being right, even on matters which may be more about taste and preference than anything that can be proven mathematically. Worse, you insist on trying to convince people even when it might be better to just give up and decide that maybe something not's worth the effort to get the last word in. This is how we get to centithreads. If every time someone disagrees, you insist on replying, and then if people give up in disgust, you then try to use that as proof that you must be right, since you've dazzled them with your brilliance, that's not good for the development community. Sometimes a question is important enough that it's worth doing this. But I'd suggest to you that you should ask yourself whether you're doing it too often. After all, this is open source. If you are convinced that you are right, and everyone else in the community is wrong, it is within your power to fork the code base, and to prove us wrong by creating a better product. Or you can decide to just keep a patch set to yourself, or perhaps post it periodically, if it is some key feature that you are certain you or your company can't live with out. Heck, I've done this with ext4, even though I'm the maintainer --- there have been features that I know are critical for my company, but the rest of the ext4 development community are stridently against. I've just simply posted the patch set once, and if it gets strongly opposed, I'll just keep it as a out-of-tree patch. The fallocate NO_HIDE_STALE flag is a good example of that; it's used in production on thousands and thousands of servers by Google and Tao Bao, but since there was strong opposition on the ext4 list, we've kept it as an out-of-tree patch. Note what I did not do. I did not force the patch in, even though it might be within my power as the maintainer; nor did I try to convince people over and OVER and OVER again about the rightness of my position, and turn it into a centithread. My claim is that all I did was disagree with Junio. You can invalidate that claim easily by providing *a single* example where I did more than disagree. The problem is when you disagree with a number of people (not just Junio), and you are, in my opinion, overly persistent. We can argue whether you've stepped over the line in terms of impugning people's motives or sanity, but that's not necessarily the most important issue. People sometimes step over the line, and while that's unfortunate, it's when it becomes a persistent pattern, and it happens frequently enough, that it becomes a real problem. Alternatively, if you are right that Junio is mad, and he's being capriciously insulting, then I'm sure that when you establish your own fork, lots of developers will come flocking to your flag. If they do not, then perhaps you might want to take that as some objective evidence that the community is perhaps, more willing to work with him, then they are to work with you. Best regards, - Ted P.S. There are plenty of things that I consider to be unfortunate about git's command line interface, in terms of inconsistencies and confusing terminology. Over the past 5+ years, I've observed that I think the way commit selection in git format-patch is inconsistent with how we handle commit selection for other commands, e.g., git log commit vs and git format-patch commit. Even if you think that this is a matter of self-inherent truth, versus just a matter of taste, there is also the consideration of backwards compatibility, and the question of how important consistency and easy of learning gets traded off against backwards compatibility and invalidating potentially huge numbers of shell scripts and documentation. So it's not something where I've made a nuisance of myself, because it's a settled issue. As another example, people have agreed for a long time that the fact that tab characters are significant in Makefiles is highly unfortunate. However, no one is running around calling the GNU Make maintainers insane for not being willing to make a change that would break huge numbers of Makefiles in the world. More importantly, people aren't brining up the same subject over and over and over again on the GNU Makefile mailing list. Perhaps you might consider what would be the appropriate response if someone insisteted on creating centithreads on the GNU Makefile discuss list on that subject.
Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Fri, Oct 18, 2013 at 4:00 PM, Duy Nguyen pclo...@gmail.com wrote: On Fri, Oct 18, 2013 at 8:55 PM, Duy Nguyen pclo...@gmail.com wrote: On Fri, Oct 18, 2013 at 8:17 PM, Johan Herland jo...@herland.net wrote: diff --git a/sha1_file.c b/sha1_file.c index f80bbe4..00e 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -2857,7 +2857,9 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename) /* Make sure the directory exists */ memcpy(buffer, filename, dirlen); buffer[dirlen-1] = 0; - if (mkdir(buffer, 0777) || adjust_shared_perm(buffer)) + if (mkdir(buffer, 0777) errno != EEXIST) + return -1; + if (adjust_shared_perm(buffer)) return -1; I was going to ask what if the created directory does not have permission 0777. But adjust_shared_perm follows immediately after, so we're good. And I sent too early. adjust_shared_perm() does rely on current dir's permission. So if the creator does mkdir(buffer, 0), we still have a bad permission in the end. And adjust_shared_perm() is only active when the repository is configured to be shared. I'm not sure where you're going with this... Whatever happens in another process between the first call git_mkstemp_mode() and our mkdir() call, one of three things will happen in this code: (a) mkdir() succeeds, i.e. we won the race. This case is unchanged by my patch. (b) mkdir() fails with errno != EEXIST. This results in us returning -1 to our caller. This case is also unchanged by my patch. (c) mkdir() fails with EEXIST, i.e. we lost the race. We do the adjust_shared_perms() which might fail (- abort) or succeed, and we then _retry_ the git_mkstemp_mode(). There is no case where the whatever that happened between the first git_mkstemp_mode() and mkdir() will have a different outcome (create_tmpfile() failing or suceeding) than if it had happened _before_ the first git_mkstemp_mode(). And it is not our responsibility to control what other/unrelated processes might end up doing with directories inside .git/objects/... What am I missing? ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Comments on Git
I've just completed a task involving two releases of a piece of embedded software and an attempt to integrate portions of one into the other. My comments: Git is just *ucking incredible! Very well done people... Nick -- To unsubscribe from this list: send the line unsubscribe 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] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Fri, Oct 18, 2013 at 10:52 AM, Johan Herland jo...@herland.net wrote: On Fri, Oct 18, 2013 at 3:53 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Fri, Oct 18, 2013 at 9:17 AM, Johan Herland jo...@herland.net wrote: There are cases (e.g. when running concurrent fetches in a repo) where multiple Git processes concurrently attempt to create loose objects within the same objects/XX/ dir. The creation of the loose object files is (AFAICS) safe from races, but the creation of the objects/XX/ dir in which the loose objects reside is unsafe, for example: [...] The fix is to simply handle mkdir() setting errno = EEXIST as success. Would it make sense for the commit message to explain what happens if EEXIST is returned but the pathname is not a directory? (For instance, in the same race window, another process had created a plain file or pipe there.) I'm pretty sure in that case, the following adjust_shared_perm() or git_mkstemp_mode() would fail, and we'd end up returning error from create_tmpfile() in any case. From a quick glance at the code, that was my impression as well. I can add the above to the commit message if you insist. :) It was the only unanswered question which popped into my mind upon reading the commit message, and required code consultation to answer. A benefit of mentioning it in the commit message is that future readers see that these other cases were taken into consideration (but I don't insist upon a re-roll). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: My patches
Theodore Ts'o wrote: On Fri, Oct 18, 2013 at 06:41:41AM -0500, Felipe Contreras wrote: And I hazard to guess that the vast majority agree with Junio on this (based, again, on email evidence). Not with you. That is irrelevant, and a fallacy. The vast majority of people thought the Earth was the center of the universe, and they were all wrong. It's called ad populum fallacy, look it up. Wether the majority of Git developers agree that there's something more than a disagreement is irrelevant, their opinion doesn't change the truth. Look, the problem is that you insist on being right, even on matters which may be more about taste and preference than anything that can be proven mathematically. I don't insist on being right, I have an opinion and I voice it, there is nothing wrong with that. If the other side agrees there's a difference of opinion, that's the end of the discussion. I would say it's actually the other side that insists on being right, and that's the problem; they don't agree it's a difference in opinion, from their point of one side is right, and the other side is wrong, and that's what causes their frustration. Ask Junio if he thinks it's simply a matter of a difference in opinion. He pretty much already said it's not. Worse, you insist on trying to convince people even when it might be better to just give up and decide that maybe something not's worth the effort to get the last word in. This is how we get to centithreads. If every time someone disagrees, you insist on replying, This is how it goes: 1) Side A 2) Side B 3) Side A 4) Side B 5) Side A 6) Side B At any point in time side B could stop replying, sure, but so could side A. Why do you blame ME for replying, and not the other side, for replying to my reply? Presumably because right before reply 4), side A thought the discussion was wortwhile, and something happened in 5) that changed their opinion, and now side B becomes a problematic person. And since you are friends with side A, you take their side. and then if people give up in disgust, you then try to use that as proof that you must be right, Show me *one* instance when I have done so. I have never used silence as evidence of anything. Sometimes a question is important enough that it's worth doing this. But I'd suggest to you that you should ask yourself whether you're doing it too often. After all, this is open source. If you are convinced that you are right, and everyone else in the community is wrong, it is within your power to fork the code base, and to prove us wrong by creating a better product. Don't worry, that is *exactly* what I plan to do. The fallocate NO_HIDE_STALE flag is a good example of that; it's used in production on thousands and thousands of servers by Google and Tao Bao, but since there was strong opposition on the ext4 list, we've kept it as an out-of-tree patch. Note what I did not do. I did not force the patch in, even though it might be within my power as the maintainer; nor did I try to convince people over and OVER and OVER again about the rightness of my position, and turn it into a centithread. My patches are not good just for me or my company, they are good for everyone. Have you actually looked at any of them? My claim is that all I did was disagree with Junio. You can invalidate that claim easily by providing *a single* example where I did more than disagree. The problem is when you disagree with a number of people (not just Junio), and you are, in my opinion, overly persistent. But that's not what Junio said. This is the second time you defend Junio by assuming his position is exactly the opposite. Junio doesn't think it's just a disagreement, Junio doesn't think I'm just being persistent, Junio is saying I can't be worked with. The interesting thing is that when Junio agrees with the change, he can work with me, when I agree my change is not good, the same, but suddenly when I don't agree, then I'm not good to work with. See the pattern? We can argue whether you've stepped over the line in terms of impugning people's motives or sanity, but that's not necessarily the most important issue. People sometimes step over the line, and while that's unfortunate, it's when it becomes a persistent pattern, and it happens frequently enough, that it becomes a real problem. Have I ever impugned people's motives or sanity? Please, show me where I did that. Alternatively, if you are right that Junio is mad, I didn't say Junio is mad, I said he got mad. : carried away by intense anger : furious mad about the delay http://www.merriam-webster.com/dictionary/mad and he's being capriciously insulting, then I'm sure that when you establish your own fork, lots of developers will come flocking to your flag. If they do not, then perhaps you might want to take that as some objective evidence that the community is perhaps, more willing to work with him,
Re: My patches
Theodore Ts'o ty...@mit.edu writes: Over the past 5+ years, I've observed that I think the way commit selection in git format-patch is inconsistent with how we handle commit selection for other commands, e.g., git log commit vs and git format-patch commit. Even if you think that this is a matter of self-inherent truth, versus just a matter of taste, there is also the consideration of backwards compatibility, and the question of how important consistency and easy of learning gets traded off against backwards compatibility and invalidating potentially huge numbers of shell scripts and documentation. So it's not something where I've made a nuisance of myself, because it's a settled issue. The original syntax to select of commits by format-patch is very inconsistent from the log family because it was done way before the log family's way has been established as the best practice. It has annoyed enough people that we spent effort to teach recent Git to accept $ git format-patch master..next as well. So it indeed is a settled issue, but you are correct to point out that we had to find a way to do so while still keeping the original syntax working for people who have scripts and people who work from random and stale documents we have not much control over updating. -- To unsubscribe from this list: send the line 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] remote-hg: unquote C-style paths when exporting
git-fast-import documentation says that paths can be C-style quoted.
Unfortunately, the current remote-hg helper doesn't unquote quoted
path and pass them as-is to Mercurial when the commit is created.
This result in the following situation:
- clone a mercurial repository with git
- Add a file with space: `mkdir dir/foo\ bar`
- Commit that new file, and push the change to mercurial
- The mercurial repository as now a new directory named 'dir', which
contains a file named 'foo bar'
Use python ast.literal_eval to unquote the string if it starts with .
It has been tested with quotes, spaces, and utf-8 encoded file-names.
Signed-off-by: Antoine Pelisse apeli...@gmail.com
---
contrib/remote-helpers/git-remote-hg | 3 +++
1 file changed, 3 insertions(+)
diff --git a/contrib/remote-helpers/git-remote-hg
b/contrib/remote-helpers/git-remote-hg
index 92d994e..0141949 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -14,6 +14,7 @@
from mercurial import hg, ui, bookmarks, context, encoding, node, error,
extensions, discovery, util
+import ast
import re
import sys
import os
@@ -742,6 +743,8 @@ def parse_commit(parser):
f = { 'deleted' : True }
else:
die('Unknown file command: %s' % line)
+if path.startswith(''):
+path = ast.literal_eval(path)
files[path] = f
# only export the commits if we are on an internal proxy repo
--
1.8.4.1.507.g9768648.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] http: update base URLs when we see redirects
Jeff King p...@peff.net writes: + * Our basic strategy is to compare base and asked to find the bits + * specific to our request. We then strip those bits off of got to yield the + * new base. So for example, if our base is http://example.com/foo.git;, + * and we ask for http://example.com/foo.git/info/refs;, we might end up + * with https://other.example.com/foo.git/info/refs;. We would want the + * new URL to become https://other.example.com/foo.git;. Not https://other.example.com/foo.git/info/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: [PATCH 6/9] http: update base URLs when we see redirects
On Fri, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: + * Our basic strategy is to compare base and asked to find the bits + * specific to our request. We then strip those bits off of got to yield the + * new base. So for example, if our base is http://example.com/foo.git;, + * and we ask for http://example.com/foo.git/info/refs;, we might end up + * with https://other.example.com/foo.git/info/refs;. We would want the + * new URL to become https://other.example.com/foo.git;. Not https://other.example.com/foo.git/info/refs;? I think my use of the new URL is ambiguous. I meant the new base, from which one could then construct the new refs URL as you suggest. -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] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Fri, Oct 18, 2013 at 05:41:54PM +0200, Johan Herland wrote: (c) mkdir() fails with EEXIST, i.e. we lost the race. We do the adjust_shared_perms() which might fail (- abort) or succeed, and we then _retry_ the git_mkstemp_mode(). There is no case where the whatever that happened between the first git_mkstemp_mode() and mkdir() will have a different outcome (create_tmpfile() failing or suceeding) than if it had happened _before_ the first git_mkstemp_mode(). I think your (c) is fine as long as adjust_shared_perms() is idempotent, as we will run it twice (one for each side of the race). But from my reading, I think it is. I was almost tempted to say we do not even need to run adjust_shared_perm twice, but we do, or we risk another race: one side loses the mkdir race, but wins the open() race, and writes to a wrong-permission directory. Of course, that should not matter unless the racers are two different users (in a shared repo), and in that case, we wins the adjust_shared_perm race, but does not have permission to change the mode. And it is not our responsibility to control what other/unrelated processes might end up doing with directories inside .git/objects/... Agreed. We already leave a wrong-permission directory in place if it existed before we started create_tmpfile. The code before your patch, when racing with such a wrong-directory creator, would abort the tmpfile. Now it will correct the permissions. Either behavior seems fine to me (yours actually seems better, but the point is that it does not matter because they are dwarfed by the non-race cases where the directory is already sitting 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: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Karsten Blees karsten.bl...@gmail.com writes:
The coredumps are caused by my patch #10, which free()s
cache_entries when they are removed, in combination with ...
Looking at that patch, it makes me wonder if remove_index_entry_at()
and replace_index_entry() should be the ones that frees the old
entry in the first place. A caller may already have a ce pointing
at an old entry and use the information from old_ce to update a new
one after it installed it, e.g.
old_ce = ...
new_ce = make_cache_entry(... old_ce-name, ...);
replace_index_entry(... new_ce);
new_ce-ce_mode = old_ce-cd_mode;
free(old_ce);
The same goes for the functions that remove the entry.
But I am probably biased saying this, because in the old days, cache
entries could never be freed (they were carved out of a contiguous
region of memory, mmapped from the index file). These days, we
parse and run ntoh*() on the on-disk cache entries to create in-core
form, and the cache entries should never be freed is no longer
true, but I would not be surprised if there are still some code
leftover that relies on use after free being safe, leaking unused
cache entries.
Going forward, I do agree with your patch #10 that removal or
replacing that may make an existing entry unreferenced should free
entries that are no longer used, and use after free should be
forbidden.
Can't we just use add_file_to_cache here (which replaces
cache_entries by creating a copy)?
diff --git a/submodule.c b/submodule.c
index 1905d75..e388487 100644
--- a/submodule.c
+++ b/submodule.c
@@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
void stage_updated_gitmodules(void)
{
- struct strbuf buf = STRBUF_INIT;
- struct stat st;
- int pos;
- struct cache_entry *ce;
- int namelen = strlen(.gitmodules);
-
- pos = cache_name_pos(.gitmodules, namelen);
- if (pos 0) {
- warning(_(could not find .gitmodules in index));
- return;
- }
I think the remainder is (morally) equivalent between the original
and a single add-file-to-cache call, and the version after your
how about this patch in the message I am responding to looks more
correct (e.g. why does the original lstat after it has read the
file?).
But this warning may want to stay, no?
- ce = active_cache[pos];
- ce-ce_flags = namelen;
- if (strbuf_read_file(buf, .gitmodules, 0) 0)
- die(_(reading updated .gitmodules failed));
- if (lstat(.gitmodules, st) 0)
- die_errno(_(unable to stat updated .gitmodules));
- fill_stat_cache_info(ce, st);
- ce-ce_mode = ce_mode_from_stat(ce, st.st_mode);
- if (remove_cache_entry_at(pos) 0)
- die(_(unable to remove .gitmodules from index));
- if (write_sha1_file(buf.buf, buf.len, blob_type, ce-sha1))
- die(_(adding updated .gitmodules failed));
- if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
+ if (add_file_to_cache(.gitmodules, 0))
die(_(staging updated .gitmodules failed));
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] http: update base URLs when we see redirects
Jeff King p...@peff.net writes: On Fri, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: + * Our basic strategy is to compare base and asked to find the bits + * specific to our request. We then strip those bits off of got to yield the + * new base. So for example, if our base is http://example.com/foo.git;, + * and we ask for http://example.com/foo.git/info/refs;, we might end up + * with https://other.example.com/foo.git/info/refs;. We would want the + * new URL to become https://other.example.com/foo.git;. Not https://other.example.com/foo.git/info/refs;? I think my use of the new URL is ambiguous. I meant the new base, from which one could then construct the new refs URL as you suggest. Ahh, there is nothing we need to do to make the new URL to https://.../info/refs;; we already have it in got. And the function resets base and then adds got excluding its tail part to compute the new base. So s/new URL/new base/ would be all we need to do, I think. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
Jeff King p...@peff.net writes: I was almost tempted to say we do not even need to run adjust_shared_perm twice, but we do, or we risk another race: one side loses the mkdir race, but wins the open() race, and writes to a wrong-permission directory. Of course, that should not matter unless the racers are two different users (in a shared repo), and in that case, we wins the adjust_shared_perm race, but does not have permission to change the mode. Interesting. Agreed. We already leave a wrong-permission directory in place if it existed before we started create_tmpfile. The code before your patch, when racing with such a wrong-directory creator, would abort the tmpfile. Now it will correct the permissions. Either behavior seems fine to me (yours actually seems better, but the point is that it does not matter because they are dwarfed by the non-race cases where the directory is already sitting there). Agreed. We may notice the failure to correct the permissions in the new code, where the old code left existing directories incorrect permissions as they were. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Am 18.10.2013 02:42, schrieb Karsten Blees:
Am 17.10.2013 23:07, schrieb Junio C Hamano:
Junio C Hamano gits...@pobox.com writes:
Karsten Blees karsten.bl...@gmail.com writes:
Am 16.10.2013 23:43, schrieb Junio C Hamano:
* kb/fast-hashmap (2013-09-25) 6 commits
- fixup! diffcore-rename.c: simplify finding exact renames
- diffcore-rename.c: use new hash map implementation
- diffcore-rename.c: simplify finding exact renames
- diffcore-rename.c: move code around to prepare for the next patch
- buitin/describe.c: use new hash map implementation
- add a hashtable implementation that supports O(1) removal
I posted a much more complete v3 [1], but somehow missed Jonathan's fixup!
commit.
Thanks; I'll replace the above with v3 and squash the fix-up in.
Interestingly, v3 applied on 'maint' and then merged to 'master'
seems to break t3600 and t7001 with a coredump.
It would conflict with es/name-hash-no-trailing-slash-in-dirs that
has been cooking in 'next', too; the resolution might be trivial but
I didn't look too deeply into it.
I've pushed a rebased version to
https://github.com/kblees/git/commits/kb/hashmap-v3-next
(no changes yet except for Jonathan's fixup in #04 and merge resolution).
The coredumps are caused by my patch #10, which free()s cache_entries when
they are removed, in combination with submodule.c::stage_updated_gitmodules
(5fee9952 submodule.c: add .gitmodules staging helper functions), which
removes a cache_entry, then modifies and re-adds the (now) free()d memory.
Can't we just use add_file_to_cache here (which replaces cache_entries by
creating a copy)?
No objections from my side. Looks like we could also copy the
cache entry just before remove_cache_entry_at() and use that
copy afterwards, but your solution is so much shorter that I
would really like to use it (unless someone more cache-savvy
than me has any objections).
And by the way: this is the last use of remove_cache_entry_at(),
would it make sense to remove that define while at it? Only the
remove_index_entry_at() function it is defined to is currently
used.
diff --git a/submodule.c b/submodule.c
index 1905d75..e388487 100644
--- a/submodule.c
+++ b/submodule.c
@@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
void stage_updated_gitmodules(void)
{
- struct strbuf buf = STRBUF_INIT;
- struct stat st;
- int pos;
- struct cache_entry *ce;
- int namelen = strlen(.gitmodules);
-
- pos = cache_name_pos(.gitmodules, namelen);
- if (pos 0) {
- warning(_(could not find .gitmodules in index));
- return;
- }
- ce = active_cache[pos];
- ce-ce_flags = namelen;
- if (strbuf_read_file(buf, .gitmodules, 0) 0)
- die(_(reading updated .gitmodules failed));
- if (lstat(.gitmodules, st) 0)
- die_errno(_(unable to stat updated .gitmodules));
- fill_stat_cache_info(ce, st);
- ce-ce_mode = ce_mode_from_stat(ce, st.st_mode);
- if (remove_cache_entry_at(pos) 0)
- die(_(unable to remove .gitmodules from index));
- if (write_sha1_file(buf.buf, buf.len, blob_type, ce-sha1))
- die(_(adding updated .gitmodules failed));
- if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
+ if (add_file_to_cache(.gitmodules, 0))
die(_(staging updated .gitmodules failed));
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Am 18.10.2013 21:09, schrieb Junio C Hamano:
Karsten Blees karsten.bl...@gmail.com writes:
Can't we just use add_file_to_cache here (which replaces
cache_entries by creating a copy)?
diff --git a/submodule.c b/submodule.c
index 1905d75..e388487 100644
--- a/submodule.c
+++ b/submodule.c
@@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
void stage_updated_gitmodules(void)
{
- struct strbuf buf = STRBUF_INIT;
- struct stat st;
- int pos;
- struct cache_entry *ce;
- int namelen = strlen(.gitmodules);
-
- pos = cache_name_pos(.gitmodules, namelen);
- if (pos 0) {
- warning(_(could not find .gitmodules in index));
- return;
- }
I think the remainder is (morally) equivalent between the original
and a single add-file-to-cache call, and the version after your
how about this patch in the message I am responding to looks more
correct (e.g. why does the original lstat after it has read the
file?).
Cargo cult programming. I was looking at other code manipulating
the index (as Documentation/technical/api-in-core-index.txt is
rather terse ;-) and concluded I would need to read the possibly
updated st.st_mode, in case updating the config file would have
changed that.
But this warning may want to stay, no?
Of course you are right on this one. All test ran successfully
with this patch, so I think adding one for that warning makes
sense too. And as that is submodule related stuff I volunteer
for fixing all this ;-)
- ce = active_cache[pos];
- ce-ce_flags = namelen;
- if (strbuf_read_file(buf, .gitmodules, 0) 0)
- die(_(reading updated .gitmodules failed));
- if (lstat(.gitmodules, st) 0)
- die_errno(_(unable to stat updated .gitmodules));
- fill_stat_cache_info(ce, st);
- ce-ce_mode = ce_mode_from_stat(ce, st.st_mode);
- if (remove_cache_entry_at(pos) 0)
- die(_(unable to remove .gitmodules from index));
- if (write_sha1_file(buf.buf, buf.len, blob_type, ce-sha1))
- die(_(adding updated .gitmodules failed));
- if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
+ if (add_file_to_cache(.gitmodules, 0))
die(_(staging updated .gitmodules failed));
--
To unsubscribe from this list: send the line 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-svn documentation: Use tabs consistently within the ascii doc
While I can understand 4 or 7 white spaces are fancy, we'd rather want to use tabs throughout the whole document. Signed-off-by: Stefan Beller stefanbel...@googlemail.com --- Documentation/git-svn.txt | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt index 2a38476..58578ad 100644 --- a/Documentation/git-svn.txt +++ b/Documentation/git-svn.txt @@ -175,11 +175,11 @@ Skip branches and tags of first level directories;; precedence over '--include-paths'. --log-window-size=n;; -Fetch n log entries per request when scanning Subversion history. -The default is 100. For very large Subversion repositories, larger -values may be needed for 'clone'/'fetch' to complete in reasonable -time. But overly large values may lead to higher memory usage and -request timeouts. + Fetch n log entries per request when scanning Subversion history. + The default is 100. For very large Subversion repositories, larger + values may be needed for 'clone'/'fetch' to complete in reasonable + time. But overly large values may lead to higher memory usage and + request timeouts. 'clone':: Runs 'init' and 'fetch'. It will automatically create a @@ -366,12 +366,12 @@ environment). This command has the same behaviour. Any other arguments are passed directly to 'git log' 'blame':: - Show what revision and author last modified each line of a file. The - output of this mode is format-compatible with the output of - `svn blame' by default. Like the SVN blame command, - local uncommitted changes in the working tree are ignored; - the version of the file in the HEAD revision is annotated. Unknown - arguments are passed directly to 'git blame'. + Show what revision and author last modified each line of a file. The + output of this mode is format-compatible with the output of + `svn blame' by default. Like the SVN blame command, + local uncommitted changes in the working tree are ignored; + the version of the file in the HEAD revision is annotated. Unknown + arguments are passed directly to 'git blame'. + --git-format;; Produce output in the same format as 'git blame', but with -- 1.8.4.1.511.g52c26ce -- To unsubscribe from this list: send the line 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] submodule: don't access the .gitmodules cache entry after removing it
Commit 5fee995244e introduced the stage_updated_gitmodules() function to
add submodule configuration updates to the index. It assumed that even
after calling remove_cache_entry_at() the same cache entry would still be
valid. This was true in the old days, as cache entries could never be
freed, but that is not so sure in the present as there is ongoing work to
free removed cache entries, which makes this code segfault.
Fix that by calling add_file_to_cache() instead of open coding it. Also
remove the could not find .gitmodules in index warning, as that won't
happen in regular use cases (and by then just silently adding it to the
index we do the right thing).
Thanks-to: Karsten Blees karsten.bl...@gmail.com
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
Am 18.10.2013 21:52, schrieb Jens Lehmann:
Am 18.10.2013 21:09, schrieb Junio C Hamano:
Karsten Blees karsten.bl...@gmail.com writes:
Can't we just use add_file_to_cache here (which replaces
cache_entries by creating a copy)?
diff --git a/submodule.c b/submodule.c
index 1905d75..e388487 100644
--- a/submodule.c
+++ b/submodule.c
@@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
void stage_updated_gitmodules(void)
{
- struct strbuf buf = STRBUF_INIT;
- struct stat st;
- int pos;
- struct cache_entry *ce;
- int namelen = strlen(.gitmodules);
-
- pos = cache_name_pos(.gitmodules, namelen);
- if (pos 0) {
- warning(_(could not find .gitmodules in index));
- return;
- }
But this warning may want to stay, no?
Of course you are right on this one. All test ran successfully
with this patch, so I think adding one for that warning makes
sense too. And as that is submodule related stuff I volunteer
for fixing all this ;-)
And after digging deeper and trying to come up with a test
case for that I think we can safely remove this warning too.
When not having a .gitmodules file update_path_in_gitmodules()
and remove_path_from_gitmodules() won't do anything and signal
that there is no need for stage_updated_gitmodules(). And if
we get a user later that wants to create .gitmodules file (e.g.
to register a new submodule) a warning that that file had not
been known before isn't helpful. So the only cases triggering
that warning would be when the .gitmodules is on the filesystem
but not in the index, and I can't think of a regular use case
where this happens. So let's drop it too.
submodule.c | 25 +
1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/submodule.c b/submodule.c
index 1905d75..e388487 100644
--- a/submodule.c
+++ b/submodule.c
@@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
void stage_updated_gitmodules(void)
{
- struct strbuf buf = STRBUF_INIT;
- struct stat st;
- int pos;
- struct cache_entry *ce;
- int namelen = strlen(.gitmodules);
-
- pos = cache_name_pos(.gitmodules, namelen);
- if (pos 0) {
- warning(_(could not find .gitmodules in index));
- return;
- }
- ce = active_cache[pos];
- ce-ce_flags = namelen;
- if (strbuf_read_file(buf, .gitmodules, 0) 0)
- die(_(reading updated .gitmodules failed));
- if (lstat(.gitmodules, st) 0)
- die_errno(_(unable to stat updated .gitmodules));
- fill_stat_cache_info(ce, st);
- ce-ce_mode = ce_mode_from_stat(ce, st.st_mode);
- if (remove_cache_entry_at(pos) 0)
- die(_(unable to remove .gitmodules from index));
- if (write_sha1_file(buf.buf, buf.len, blob_type, ce-sha1))
- die(_(adding updated .gitmodules failed));
- if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
+ if (add_file_to_cache(.gitmodules, 0))
die(_(staging updated .gitmodules failed));
}
--
1.8.4.1.545.gc07df9e
--
To unsubscribe from this list: send the line unsubscribe 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-users] Problem using detached worktrees with commands implemented in scripts
From: Jonathan Nieder jrnie...@gmail.com Philip Oakley wrote: It was Dale that had the problem, I was just suggesting where he might want to look... ;-) A bit more looking gave that the cd_to_toplevel () in git-sh-setup.sh directly uses `git rev-parse --show-toplevel`, which simply returns work_tree (static char *work_tree; in environment.c, with comment /* This is set by setup_git_dir_gently() and/or git_default_config() */), apparently without a check for the GIT_WORK_TREE. Getting closer. :) The usual way to use GIT_WORK_TREE is along with GIT_DIR. When re-checking the manual's git(1) env variable section the comment that implies this didn't read well The value will not be used in combination The section probably needs that to be stated explicitly (an exported GIT_WORK_TREE is ignored if GIT_DIR is not set). That takes you into the setup_explicit_git_dir() codepath, which does respect GIT_WORK_TREE as it should. (setup_discovered_git_dir does, too.) The strange behavior you ran into is that unlike, say, git-pull.sh and git-am.sh, filter-branch does not set SUBDIRECTORY_OK, store the prefix from 'git rev-parse --show-prefix', and then cd_to_toplevel at the top of the script. In other words, nobody bothered to make it work from anywhere other than the toplevel of the worktree to begin with, and nobody wanted it enough to fix it later. I maybe wrong, but I thought that in Dale's case he was already at the same level as the GIT_WORK_TREE he had set, so may not have expected to need a cd_to_toplevel Dale did propose a patch in http://thread.gmane.org/gmane.comp.version-control.git/236260 as the error was from later in the setup script. Hope that helps, Jonathan Philip -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Oct 2013, #04; Fri, 18)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to master] * es/name-hash-no-trailing-slash-in-dirs (2013-09-17) 4 commits (merged to 'next' on 2013-09-20 at 9633d9a) + dir: revert work-around for retired dangerous behavior + name-hash: stop storing trailing '/' on paths in index_state.dir_hash + employ new explicit exists in index? API + name-hash: refactor polymorphic index_name_exists() Clean up the internal of the name-hash mechanism used to work around case insensitivity on some filesystems to cleanly fix a long-standing API glitch where the caller of cache_name_exists() that ask about a directory with a counted string was required to have '/' at one location past the end of the string. * jc/checkout-detach-doc (2013-09-11) 1 commit (merged to 'next' on 2013-09-17 at 438cf13) + checkout: update synopsys and documentation on detaching HEAD git checkout [--detach] commit was listed poorly in the synopsis section of its documentation. * jc/reflog-doc (2013-06-19) 1 commit (merged to 'next' on 2013-09-25 at 4eb0c14) + setup_reflog_action: document the rules for using GIT_REFLOG_ACTION Document rules to use GIT_REFLOG_ACTION variable in the scripted Porcelain. git-rebase--interactive locally violates them, but it is a leaf user that does not call out to or dot-source other scripts, so it does not urgently need to be fixed. * jk/clone-progress-to-stderr (2013-09-18) 3 commits (merged to 'next' on 2013-09-25 at 137af9e) + clone: always set transport options + clone: treat checking connectivity like other progress + clone: send diagnostic messages to stderr Some progress and diagnostic messages from git clone were incorrectly sent to the standard output stream, not to the standard error stream. * jk/format-patch-from (2013-09-20) 1 commit (merged to 'next' on 2013-09-20 at 0506530) + format-patch: print in-body From only when needed format-patch --from=whom forgot to omit unnecessary in-body from line, i.e. when whom is the same as the real author. * jk/trailing-slash-in-pathspec (2013-09-13) 2 commits (merged to 'next' on 2013-09-17 at 18fe277) + reset: handle submodule with trailing slash + rm: re-use parse_pathspec's trailing-slash removal Code refactoring. * lc/filter-branch-too-many-refs (2013-09-12) 1 commit (merged to 'next' on 2013-09-17 at 31cd01a) + Allow git-filter-branch to process large repositories with lots of branches. git filter-branch in a repository with many refs blew limit of command line length. * sb/repack-in-c (2013-09-17) 3 commits (merged to 'next' on 2013-09-25 at 7c47036) + repack: improve warnings about failure of renaming and removing files + repack: retain the return value of pack-objects + repack: rewrite the shell script in C Rerolled, and I think it is in a reasonably good shape. -- [New Topics] * mm/checkout-auto-track-fix (2013-10-18) 2 commits - checkout: proper error message on 'git checkout foo bar --' - checkout: allow dwim for branch creation for git checkout $branch -- git checkout topic, when there is not yet a local topic branch but there is a unique remote-tracking branch for a remote topic branch, pretended as if git checkout -t -b topic remote/$r/topic (for that unique remote $r) was run. This hack however was not implemented for git checkout topic --. Will merge to 'next'. * hn/log-graph-color-octopus (2013-10-18) 1 commit - graph: fix coloring around octopus merges Will merge to 'next'. * nd/gc-lock-against-each-other (2013-10-18) 1 commit - gc: remove gc.pid file at end of execution Will merge to 'next'. -- [Stalled] * np/pack-v4 (2013-09-18) 90 commits - packv4-parse.c: add tree offset caching - t1050: replace one instance of show-index with verify-pack - index-pack, pack-objects: allow creating .idx v2 with .pack v4 - unpack-objects: decode v4 trees - unpack-objects: allow to save processed bytes to a buffer - ... Nico and Duy advancing the eternal vaporware pack-v4. This is here primarily for wider distribution of the preview edition. * sc/doc-howto-dumb-http (2013-10-16) 1 commit . doc/howto: warn about (dumb)http server document being too old The new text needs to go somewhere in the body of the document, not before the title line. * tr/merge-recursive-index-only (2013-07-07) 3 commits - merge-recursive: -Xindex-only to leave worktree unchanged - merge-recursive: untangle double meaning of o-call_depth - merge-recursive: remove dead conditional in update_stages() Holding until there is a caller to learn from.
Re: [PATCH v2 2/2] Update documentation for http.continue option
On Sat, Oct 12, 2013 at 12:26:39AM +, brian m. carlson wrote: On Fri, Oct 11, 2013 at 04:50:52PM -0700, Jonathan Nieder wrote: Perhaps this should be conditional on the authentication method used, so affected people could still contact broken servers without having different configuration per server and without having to wait a second for the timeout. curl determines what method, but I guess it's safe to assume that the authentication method used for the probe will be the same as the one used for the final connection. Unfortunately, all curl will tell us is what was offered, not what it would have chosen, so if GSSAPI and something non-Basic are both offered, we'd have to make a guess. (curl will always prefer non-Basic to Basic for the obvious reasons.) If you can argue for some sane semantics in what we should do in that case, I'll reroll with that fix and a clearer wording for the docs. Reading Jonathan Nieder's responses to the first patch, it looks like he was going to apply it, but I haven't seen it make its way into next or pu. Junio, do you want a reroll, and if so, do you want certain semantics for autodetecting this case, or are you just looking for documentation fixes? -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [git-users] Problem using detached worktrees with commands implemented in scripts
From: Junio C Hamano gits...@pobox.com Side note: without GIT_WORK_TREE environment (or core.worktree), there is no way to tell where the top level is, so you were limited to always be at the top level of your working tree if you used GIT_DIR to refer to a repository that is not embedded in your working tree. There were some changes in this area, but I do not recall the details offhand. That's not true. The core.worktree config variable tells the top of the worktree, so once you've located the repository, you know where the worktree is. Indeed, it's not clear why GIT_WORK_TREE exists, as that allows the user to set GIT_WORK_TREE inconsistently with core.worktree. (What happens if you do that?) Dale -- To unsubscribe from this list: send the line unsubscribe 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-users] Problem using detached worktrees with commands implemented in scripts
wor...@alum.mit.edu (Dale R. Worley) writes: From: Junio C Hamano gits...@pobox.com Side note: without GIT_WORK_TREE environment (or core.worktree), there is no way to tell where the top level is, so you were limited to always be at the top level of your working tree if you used GIT_DIR to refer to a repository that is not embedded in your working tree. There were some changes in this area, but I do not recall the details offhand. That's not true. The core.worktree config variable tells the top of the worktree, so once you've located the repository, you know where the worktree is. Read the second line again, perhaps? ... it's not clear why GIT_WORK_TREE exists, ... The configuration item came _way_ later than the environment, and we need to keep users and scripts from old world working, that is why. -- To unsubscribe from this list: send the line unsubscribe 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-users] Problem using detached worktrees with commands implemented in scripts
From: Junio C Hamano gits...@pobox.com Now, when you say the cwd contains the .git directory, do you mean cd /repositories git add ../working/trees/proj-wt1/file updates file in the /repositories/proj.git/index? Or do you mean this? The pattern I use is to have this: /repository/.git /working/... then cd /repository git add /working/x/y/z works as you'd expect it to. git rm seems to work correctly under these circumstances as well. I seem to recall that using relative path values doesn't work under some conditions involving symbolic links, but I can't recall the details right now. you talk about starting Git command _outside_ the working tree (whether the working tree has its repository embedded in it is not very relevant). The above pattern is what I mean, where the cwd is not within the work tree. Dale -- To unsubscribe from this list: send the line unsubscribe 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-users] Problem using detached worktrees with commands implemented in scripts
From: Junio C Hamano gits...@pobox.com It was unclear to me which part of our documentation needs updating and how, and that was (and still is) what I was primarily interested in finding out. It seems to me that what is missing is a description of the circumstances under which Git can be run. With Subversion (the only other source control system I know in detail), the working tree that is operated on is at and below the cwd, and the working tree always points to the repository. (A subdirectory of a working tree is also a valid working tree.) With Git, it seems that the basic usage is that Git searches upward from the cwd to find the top of the work tree, which is distinguished by having a .git subdirectory. The rules when the worktree is detached are more complicated, and don't seem to be written in any single place. Dale -- To unsubscribe from this list: send the line 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] Reword repack documentation to no longer state it's a script
This updates the documentation regarding the changes introduced by a1bbc6c01 (2013-09-15, repack: rewrite the shell script in C). Signed-off-by: Stefan Beller stefanbel...@googlemail.com --- Documentation/git-repack.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt index 4c1aff6..509cf73 100644 --- a/Documentation/git-repack.txt +++ b/Documentation/git-repack.txt @@ -14,7 +14,7 @@ SYNOPSIS DESCRIPTION --- -This script is used to combine all objects that do not currently +This command is used to combine all objects that do not currently reside in a pack, into a pack. It can also be used to re-organize existing packs into a single, more efficient pack. -- 1.8.4.1.511.g52c26ce -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] Officially start moving to the term 'staging area'
Am 15.10.2013 00:29, schrieb Felipe Contreras: tl;dr: everyone except Junio C Hamano and Drew Northup agrees; we should move away from the name the index. It has been discussed many times in the past that 'index' is not an appropriate description for what the high-level user does with it, and it has been agreed that 'staging area' is the best term. I haven't followed the previous discussion, but if a final conclusion towards 'staging area' has already been reached, it should probably be revised. The term 'staging area' is more intuitive for newcomers which are more familiar with English than with Git, and it seems to be a straightforward mental notion for people with different mother tongues. In fact it is so intuitive that it's used already in a lot online documentation, and the people that do teach Git professionally use this term, because it's easier for many kinds of audiences to grasp. Such online documentation often portraits the 'staging area' as some supposedly 'unique' git feature, which I find _very_ confusing. In fact, every major SCM has a staging area. E.g. you first need to svn/hg/bzr/p4 add/remove/rename/move a file, which is somehow recorded in the working copy. These recorded changes will then be picked up by a subsequent svn/hg/bzr/p4 commit/submit. Additionally, all those systems support selectively committing individual files (by specifying the files on the commit command line or selecting them in a GUI). So git's 'unique staging area' boils down to this: 1.) Recording individual files to commit in advance (instead of specifying them at commit time). Which isn't that hard to grasp. 2.) Recording individual diff hunks or even lines to commit. Which is an advanced feature that requires even more advanced commands to be useful (i.e. stash save --keep-index; make; test; commit; stash pop). It is also entirely irrelevant to binary files (i.e. for non-technical users that use git to track documents and stuff). index: an 'index' is a guide of pointers to something else; a book index has a list of entries so the reader can locate information easily without having to go through the whole book. Git porcelain is not using the staging area to find out entries quicker; it's not an index. The 'staging area' is a sorted list of most recently checked out files, and its primary purpose is to quickly detect changes in the working copy (i.e. its an index). Of the 28 builtin main porcelain commands, 18 read the index (read_index), and 11 of those also check the state of the working copy (ie_match_stat). Incidentally, the latter include _all_ commands that update the so-called 'staging area'. Subversion until recently kept the checked out files scattered in **/.svn/text-base directories, and many operations were terribly slow due to this. Subversion 1.7 introduced a new working copy format, based on a database in the root .svn directory (i.e. an index), leading to tremendous performance improvements. The git index is a major feature that facilitates the incredible performance we're so much addicted to...why be shy about it and call it something else? stage: a 'stage' is a special area designated for convenience in order for some activity to take place; an orator would prepare a stage in order for her speak to be successful, otherwise many people might not be able to hear, or see her. Git porcelain is using the staging area precisely as a special area to be separated from the working directory for convenience. I'm not a native speaker, but in my limited understanding, 'staging' in computer jargon is the process of preparing data for a production system (i.e. until the 'stage' or 'state' of the data is ready for production). It has nothing to do with the 'stage' in a theater. I've never heard the term 'staging' beeing used for source code or software (that would be 'integration'). I've also never heard 'staging' for moving data back from a production system to some work- or development area. In any sense, 'staging' is a unidirectional process (even in a theater). If I think of the index as a staging area, it covers just a single use case: preparing new commits. With this world view, it is completely counter-intuitive that e.g. changing branches overwrites my staging area. IMO, it is ok to use 'like a staging area' when we talk about using the index to prepare new commits. However, its not ok to use 'staging area' as a general synonym for the index. Just my 2 cents Karsten -- To unsubscribe from this list: send the line unsubscribe 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] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Fri, Oct 18, 2013 at 9:24 PM, Junio C Hamano gits...@pobox.com wrote: Jeff King p...@peff.net writes: Agreed. We already leave a wrong-permission directory in place if it existed before we started create_tmpfile. The code before your patch, when racing with such a wrong-directory creator, would abort the tmpfile. Now it will correct the permissions. Either behavior seems fine to me (yours actually seems better, but the point is that it does not matter because they are dwarfed by the non-race cases where the directory is already sitting there). Agreed. We may notice the failure to correct the permissions in the new code, where the old code left existing directories incorrect permissions as they were. I'm trying to mentally construct a scenario where two writers with different configuration contexts - one with shared_repository and one without - compete in this race, and we somehow end up with one of them not being able to write its object. But the best/worst case I manage to construct is this: 1. core.sharedRepository = 0640 (group-read-only) 2. Fetch A starts running 3. core.sharedRepository = 0660 (group-writable) 4. Fetch B starts running as a non-owner (i.e. depends on group-writability) 5. One of them (doesn't matter which) wins the mkdir() race 6. A and B next enter the adjust_shared_perm() race 7. B first sets dir mode to 0660 8. A then sets dir mode to 0640 9. B now tries to write its object into the dir, but fails because it's not group-writable This case is unfortunate, but no different than if steps 3 and 4 are swapped, i.e. the case where fetch B fails because the repo is not yet group-writable. Also, remember that as part of changing core.sharedRepository like this (step 3), we also require the admin to manually alter the permission bits of existing object dirs, to make sure they are group-writable (call this step 3.5). All of this has to happen _while_ fetch A is running. Trying to code around this (frankly insane) case in the create_tmpfile()/adjust_shared_perm() code is IMHO silly. Instead, a better solution is for the admin to ensure that no fetch (or receive-pack, or other object-creating process) is running while the modification of core.sharedRepository and associated resetting of permission bits on object dirs takes place (in any case, the admin must ensure this, to resolve the possible race between an object-creating process started before the core.sharedRepository change, and the manual permission update of object dirs). ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] Officially start moving to the term 'staging area'
Karsten Blees wrote: Am 15.10.2013 00:29, schrieb Felipe Contreras: tl;dr: everyone except Junio C Hamano and Drew Northup agrees; we should move away from the name the index. It has been discussed many times in the past that 'index' is not an appropriate description for what the high-level user does with it, and it has been agreed that 'staging area' is the best term. I haven't followed the previous discussion, but if a final conclusion towards 'staging area' has already been reached, it should probably be revised. The term 'staging area' is more intuitive for newcomers which are more familiar with English than with Git, and it seems to be a straightforward mental notion for people with different mother tongues. In fact it is so intuitive that it's used already in a lot online documentation, and the people that do teach Git professionally use this term, because it's easier for many kinds of audiences to grasp. Such online documentation often portraits the 'staging area' as some supposedly 'unique' git feature, which I find _very_ confusing. In fact, every major SCM has a staging area. E.g. you first need to svn/hg/bzr/p4 add/remove/rename/move a file, which is somehow recorded in the working copy. These recorded changes will then be picked up by a subsequent svn/hg/bzr/p4 commit/submit. That is not a staging area. % hg init test % cd test % echo Hello README % hg add README % echo Bye README % hg commit -m Init % hg log -p -r -1 changeset: 0:ba28df72474c tag: tip user:Felipe Contreras felipe.contre...@gmail.com date:Fri Oct 18 19:43:42 2013 -0500 summary: Init diff -r -r ba28df72474c README --- /dev/null Thu Jan 01 00:00:00 1970 + +++ b/README Fri Oct 18 19:43:42 2013 -0500 @@ -0,0 +1,1 @@ +Bye What exactly got staged? To me the best way to think about the staging area is like a commit draft. No other VCS has anything like that. And what is the point about this argument? index: an 'index' is a guide of pointers to something else; a book index has a list of entries so the reader can locate information easily without having to go through the whole book. Git porcelain is not using the staging area to find out entries quicker; it's not an index. The 'staging area' is a sorted list of most recently checked out files, and its primary purpose is to quickly detect changes in the working copy (i.e. its an index). That is not it's primary purpose. Of the 28 builtin main porcelain commands, 18 read the index (read_index), and 11 of those also check the state of the working copy (ie_match_stat). Incidentally, the latter include _all_ commands that update the so-called 'staging area'. Subversion until recently kept the checked out files scattered in **/.svn/text-base directories, and many operations were terribly slow due to this. Subversion 1.7 introduced a new working copy format, based on a database in the root .svn directory (i.e. an index), leading to tremendous performance improvements. The git index is a major feature that facilitates the incredible performance we're so much addicted to...why be shy about it and call it something else? Tell me which subversion command adds and removes information from their working copy metadata, which is not a used as a staging area, commit draft, or even an index. Moreover, we are not discussing about Git's index file, that low level concept will stay the same, we are talking about the high level concept. stage: a 'stage' is a special area designated for convenience in order for some activity to take place; an orator would prepare a stage in order for her speak to be successful, otherwise many people might not be able to hear, or see her. Git porcelain is using the staging area precisely as a special area to be separated from the working directory for convenience. I'm not a native speaker, but in my limited understanding, 'staging' in computer jargon is the process of preparing data for a production system (i.e. until the 'stage' or 'state' of the data is ready for production). It has nothing to do with the 'stage' in a theater. It is the same. A stage in the theater is also used for preparing a production. I've never heard the term 'staging' beeing used for source code or software (that would be 'integration'). I've also never heard 'staging' for moving data back from a production system to some work- or development area. Then why are people using it in external documentation? Why is ProGit already using it? But more importantly: do you have a better name? In any sense, 'staging' is a unidirectional process (even in a theater). It is not. Props and utilites are added and removed from the stage. If I think of the index as a staging area, it covers just a single use case: preparing new commits. That is its main purpose. With this world view, it is completely
Re: [PATCH] git-svn documentation: Use tabs consistently within the ascii doc
Stefan Beller stefanbel...@googlemail.com writes: While I can understand 4 or 7 white spaces are fancy, we'd rather want to use tabs throughout the whole document. You missed lines 278 and 833. There are also some spaces around line 488, but maybe those are layout-relevant and so shouldn't be converted to tabs. -Keshav -- To unsubscribe from this list: send the line 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] Fix calling parse_pathspec with no paths nor PATHSPEC_PREFER_* flags
When parse_pathspec() is called with no paths, the behavior could be either return no paths, or return one path that is cwd. Some commands do the former, some the latter. parse_pathspec() itself does not make either the default and requires the caller to specify either flag if it may run into this situation. I've grep'd through all parse_pathspec() call sites. Some pass neither, but those are guaranteed never pass empty path to parse_pathspec(). There are two call sites that may pass empty path and are fixed with this patch. Reported-by: Antoine Pelisse apeli...@gmail.com Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- line-log.c | 3 ++- revision.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/line-log.c b/line-log.c index 8b6e497..717638b 100644 --- a/line-log.c +++ b/line-log.c @@ -760,7 +760,8 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list r = r-next; } paths[count] = NULL; - parse_pathspec(rev-diffopt.pathspec, 0, 0, , paths); + parse_pathspec(rev-diffopt.pathspec, 0, + PATHSPEC_PREFER_FULL, , paths); free(paths); } } diff --git a/revision.c b/revision.c index 0173e01..dd994e9 100644 --- a/revision.c +++ b/revision.c @@ -1372,7 +1372,8 @@ static void prepare_show_merge(struct rev_info *revs) i++; } free_pathspec(revs-prune_data); - parse_pathspec(revs-prune_data, PATHSPEC_ALL_MAGIC, 0, , prune); + parse_pathspec(revs-prune_data, PATHSPEC_ALL_MAGIC, + PATHSPEC_PREFER_FULL, , prune); revs-limited = 1; } -- 1.8.2.83.gc99314b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_file.c:create_tmpfile(): Fix race when creating loose object dirs
On Sat, Oct 19, 2013 at 01:45:03AM +0200, Johan Herland wrote: I'm trying to mentally construct a scenario where two writers with different configuration contexts - one with shared_repository and one without - compete in this race, and we somehow end up with one of them not being able to write its object. But the best/worst case I manage to construct is this: 1. core.sharedRepository = 0640 (group-read-only) 2. Fetch A starts running 3. core.sharedRepository = 0660 (group-writable) 4. Fetch B starts running as a non-owner (i.e. depends on group-writability) 5. One of them (doesn't matter which) wins the mkdir() race 6. A and B next enter the adjust_shared_perm() race 7. B first sets dir mode to 0660 8. A then sets dir mode to 0640 9. B now tries to write its object into the dir, but fails because it's not group-writable [...] Trying to code around this (frankly insane) case in the create_tmpfile()/adjust_shared_perm() code is IMHO silly. Yeah, I'd agree. You cannot just willy-nilly set core.sharedRepository per-process and expect things to work. I do not think your patch makes anything worse there. I was wondering more about the chmod call itself in adjust_shared_perms, though, even if both processes have the same settings. If we have lost the mkdir race, then the other process created the directory, and it may have a different owner, causing our chmod to fail. If we call adjust_shared_perm after an EEXIST, we are subject to this race: 1. A calls mkdir, creates directory 2. B calls mkdir, gets EEXIST 3. B calls adjust_shared_perm, which wants to set g+w. It calls chmod(), which fails (it's A's directory) 4. A calls adjust_shared_perm, which sets g+w; all is well if B now creates the object, but it already barfed after failing step 3. But if we do not call adjust_shared_perm, we are subject to this race: 1. A calls mkdir, creates directory 2. B calls mkdir, gets EEXIST 3. B tries to create object in the directory, but fails (no permission) 4. A calls adjust_shared_perm, but B has already barfed due to failure in step 3 I do not see an easy way around it. Only A can set the permissions, and B cannot continue until A has done so. So we would need some kind of synchronization (B busy-waits checking for A's chmod? Yech), or we would need to atomically create the directory with the right permissions (and group). I do not think of any of this is introduced or made worse by your patch, though. Without your patch, we do not even get as far as wondering about permissions. :) Your patch successfully handles the single-user case, but stops short of handling the multi-user case. I am not sure if it is worth trying to follow-on to fix the multi-user race, but even if we do, it would want to build on top of your patch anyway. -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
