Re: [PATCHv2] git-p4: support git worktrees
On 10 December 2016 at 21:57, Luke Diamand wrote:
> git-p4 would attempt to find the git directory using
> its own specific code, which did not know about git
> worktrees. This caused git operations to fail needlessly.
>
> Rework it to use "git rev-parse --git-dir" instead, which
> knows about worktrees.
Actually this doesn't work as well as the original version. "git
rev-parse --git-dir" won't go and find the ".git" subdirectory. The
previous version would go looking for it, so this would introduce a
regression.
Luke
>
> Signed-off-by: Luke Diamand
> ---
> git-p4.py | 47 ++-
> t/t9800-git-p4-basic.sh | 20
> 2 files changed, 46 insertions(+), 21 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..6aa8957 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -49,6 +49,13 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
> # Grab changes in blocks of this many revisions, unless otherwise requested
> defaultBlockSize = 512
>
> +def gitdir():
> +d = read_pipe("git rev-parse --git-dir").strip()
> +if not d or len(d) == 0:
> +return None
> +else:
> +return d
> +
> def p4_build_cmd(cmd):
> """Build a suitable p4 command line.
>
> @@ -562,12 +569,6 @@ def currentGitBranch():
> else:
> return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
>
> -def isValidGitDir(path):
> -if (os.path.exists(path + "/HEAD")
> -and os.path.exists(path + "/refs") and os.path.exists(path +
> "/objects")):
> -return True;
> -return False
> -
> def parseRevision(ref):
> return read_pipe("git rev-parse %s" % ref).strip()
>
> @@ -3462,7 +3463,7 @@ class P4Sync(Command, P4UserMap):
> if self.tempBranches != []:
> for branch in self.tempBranches:
> read_pipe("git update-ref -d %s" % branch)
> -os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"),
> self.tempBranchLocation))
> +os.rmdir(os.path.join(gitdir(), self.tempBranchLocation))
>
> # Create a symbolic ref p4/HEAD pointing to p4/ to allow
> # a convenient shortcut refname "p4".
> @@ -3678,23 +3679,27 @@ def main():
> (cmd, args) = parser.parse_args(sys.argv[2:], cmd);
> global verbose
> verbose = cmd.verbose
> +
> if cmd.needsGit:
> -if cmd.gitdir == None:
> -cmd.gitdir = os.path.abspath(".git")
> -if not isValidGitDir(cmd.gitdir):
> -cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
> -if os.path.exists(cmd.gitdir):
> -cdup = read_pipe("git rev-parse --show-cdup").strip()
> -if len(cdup) > 0:
> -chdir(cdup);
> -
> -if not isValidGitDir(cmd.gitdir):
> -if isValidGitDir(cmd.gitdir + "/.git"):
> -cmd.gitdir += "/.git"
> -else:
> +if cmd.gitdir:
> +os.environ["GIT_DIR"] = cmd.gitdir
> +
> +# did we get a valid git dir on the command line or via $GIT_DIR?
> +if not gitdir():
> die("fatal: cannot locate git repository at %s" % cmd.gitdir)
>
> -os.environ["GIT_DIR"] = cmd.gitdir
> +else:
> +# already in a git directory?
> +if not gitdir():
> +die("fatal: not in a valid git repository")
> +
> +cdup = read_pipe("git rev-parse --show-cdup").strip()
> +if len(cdup) > 0:
> +chdir(cdup);
> +
> +# ensure subshells spawned in the p4 repo directory
> +# get the correct GIT_DIR
> +os.environ["GIT_DIR"] = os.path.abspath(gitdir())
>
> if not cmd.run(args):
> parser.print_help()
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 0730f18..093e9bd 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -257,6 +257,26 @@ test_expect_success 'submit from detached head' '
> )
> '
>
> +test_expect_success 'submit from worktree' '
> + test_when_finished cleanup_git &&
> + git p4 clone --dest="$git" //depot &&
> + (
> + cd "$git" &&
> + git worktree add ../worktree-test
> + ) &&
> + (
> + cd "$git/../worktree-test" &&
> + test_commit "worktree-commit" &&
> + git config git-p4.skipSubmitEdit true &&
> + git p4 submit
> + ) &&
> + (
> + cd "$cli" &&
> + p4 sync &&
> + test_path_is_file worktree-commit.t
> + )
> +'
> +
> test_expect_success 'kill p4d' '
> kill_p4d
> '
> --
> 2.8.2.703.g78b384c.dirty
>
[PATCH] git-svn: document useLogAuthor and addAuthorFrom config keys
Juergen Kosel wrote: > Am 05.12.2016 um 23:54 schrieb Eric Wong: > > So, can you confirm that svn.addAuthorFrom and svn.useLogAuthor > > config keys work and can be documented? > > yes, I can confirm, that adding this configuration keys works with git > 2.1.4 work. > I have added the config keys as follows: > > git config --add --bool svn.useLogAuthor true > git config --add --bool svn.addAuthorFrom true Thanks for testing, patch below. > > Even better would be for you to provide a patch to the > > documentation :) Otherwise, I can write one up sometime this week. > > My English is not that well. So I prefer, if you update the > documentation :-) No problem, my asciidoc is a bit rusty, but I think the formatted result will be fine. (Btw, list convention here is to reply-to-all; it prevents vger from being a single-point-of-failure) ---8<--- Subject: [PATCH] git-svn: document useLogAuthor and addAuthorFrom config keys We've always supported these config keys in git-svn, so document them so users won't have to respecify them on every invocation. Reported-by: Juergen Kosel Signed-off-by: Eric Wong --- Documentation/git-svn.txt | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt index 5f9e65b0c..9bee9b0c4 100644 --- a/Documentation/git-svn.txt +++ b/Documentation/git-svn.txt @@ -664,13 +664,19 @@ creating the branch or tag. When retrieving svn commits into Git (as part of 'fetch', 'rebase', or 'dcommit' operations), look for the first `From:` or `Signed-off-by:` line in the log message and use that as the author string. ++ +[verse] +config key: svn.useLogAuthor + --add-author-from:: When committing to svn from Git (as part of 'commit-diff', 'set-tree' or 'dcommit' operations), if the existing log message doesn't already have a `From:` or `Signed-off-by:` line, append a `From:` line based on the Git commit's author string. If you use this, then `--use-log-author` will retrieve a valid author string for all commits. - ++ +[verse] +config key: svn.addAuthorFrom ADVANCED OPTIONS -- EW
Business Opportunity
I have a business opportunity that Guarantee's you 40% of $10,500,000.00 , please contact me for more details Regards, Lim Fu
Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email
Stefan Beller writes: > On Fri, Dec 9, 2016 at 3:52 PM, Junio C Hamano wrote: >> Stefan Beller writes: >> >>> So you are suggesting to >>> * have the check later in the game (e.g. just after asking >>>"Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information >>> such as additional @to @cc are available. >> >> Yeah, probably before the loop starts asking that question for each >> message. And hook does not necessarily need to cause the program to >> die. The question can be reworded to "Your hook says no, but do you >> really want to send it?", > > You could, but... Yes, "does not necessarily need to die" is different from "hence you must avoid dying". Running the hook before you start the loop to ask for each message merely makes it possible to avoid dying.
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
Vasco Almeida writes:
> I wonder why this is important when Git errors out when
> core.commentChar is set to more than 1 characters or 0 characters.
I think it should be consistent with the way core.commentchar is
treated in the rest of the system, namely this bit from config.c:
if (!strcmp(var, "core.commentchar")) {
if (!value)
return config_error_nonbool(var);
else if (!strcasecmp(value, "auto"))
auto_comment_line_char = 1;
else if (value[0] && !value[1]) {
comment_line_char = value[0];
auto_comment_line_char = 0;
} else
return error("core.commentChar should only be one
character");
return 0;
}
And I think I misread this piece of code.
We only update comment_line_char from the default "#" when the
configured value is a single-byte character and we ignore incorrect
values in the configuration file. So I think the patch you sent is
correct after all.
Re: git add -p with new file
Jeff King writes: > On Fri, Dec 09, 2016 at 01:43:24PM -0500, Ariel wrote: > ... >> But it doesn't have to be that way. You could make add -p identical to add >> without options, except the -p prompts to review diffs first. > > The question is whether you would annoy people using "-p" if you started > including untracked files by default. I agree because it's inherently an > interactive process that we can be looser with backwards compatibility. It might be interactive, but it will be irritating that we suddenly have to see hundreds of lines in an untracked file before we are asked to say "no I do not want to add this file" and have to do so for all the untracked files that happen to match the given pathspec. It might make it less irritating if one of the interactive choices offered in the first prompt were N that tells the command: "No, ignore all the untracked paths", though. I dunno. Also, because "git add -p" (no pathspec) is NOT a no-op, the similarity Ariel sees with "git add" (no other options) does not hold. As you kept explaining (but perhaps not successfully being understood yet), "add -p" works like "add -u", and it will make the command incoherent if we allowed "git add -p " to add new paths, exactly because "git add -u " does not do so. Of course that can be fixed by allowing "git add -u" to also add new paths that match pathspec. Of course, Ariel can vote for making "add -p" more like "add" (with no options) than "add -u". I _think_ it is a better way to solve the incoherence than making "add -u" to add new paths. But what it means is that "add -p " works on both tracked and untracked paths that match the given pathspec , and also "add -p" (no pathspec) must become a no-op (because "add" without option and pathspec is).
[PATCHv2] git-p4 worktree support
Second attempt at teaching git-p4 about worktrees. Earlier discussion here: http://marc.info/?l=git&m=148097985622294 Git-p4 exports GIT_DIR so that when it chdirs into the P4 client area to apply the change, git commands called from there will work correctly. Luke Diamand (1): git-p4: support git worktrees git-p4.py | 47 ++- t/t9800-git-p4-basic.sh | 20 2 files changed, 46 insertions(+), 21 deletions(-) -- 2.8.2.703.g78b384c.dirty
[PATCHv2] git-p4: support git worktrees
git-p4 would attempt to find the git directory using
its own specific code, which did not know about git
worktrees. This caused git operations to fail needlessly.
Rework it to use "git rev-parse --git-dir" instead, which
knows about worktrees.
Signed-off-by: Luke Diamand
---
git-p4.py | 47 ++-
t/t9800-git-p4-basic.sh | 20
2 files changed, 46 insertions(+), 21 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52..6aa8957 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -49,6 +49,13 @@ defaultLabelRegexp = r'[a-zA-Z0-9_\-.]+$'
# Grab changes in blocks of this many revisions, unless otherwise requested
defaultBlockSize = 512
+def gitdir():
+d = read_pipe("git rev-parse --git-dir").strip()
+if not d or len(d) == 0:
+return None
+else:
+return d
+
def p4_build_cmd(cmd):
"""Build a suitable p4 command line.
@@ -562,12 +569,6 @@ def currentGitBranch():
else:
return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
-def isValidGitDir(path):
-if (os.path.exists(path + "/HEAD")
-and os.path.exists(path + "/refs") and os.path.exists(path +
"/objects")):
-return True;
-return False
-
def parseRevision(ref):
return read_pipe("git rev-parse %s" % ref).strip()
@@ -3462,7 +3463,7 @@ class P4Sync(Command, P4UserMap):
if self.tempBranches != []:
for branch in self.tempBranches:
read_pipe("git update-ref -d %s" % branch)
-os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"),
self.tempBranchLocation))
+os.rmdir(os.path.join(gitdir(), self.tempBranchLocation))
# Create a symbolic ref p4/HEAD pointing to p4/ to allow
# a convenient shortcut refname "p4".
@@ -3678,23 +3679,27 @@ def main():
(cmd, args) = parser.parse_args(sys.argv[2:], cmd);
global verbose
verbose = cmd.verbose
+
if cmd.needsGit:
-if cmd.gitdir == None:
-cmd.gitdir = os.path.abspath(".git")
-if not isValidGitDir(cmd.gitdir):
-cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()
-if os.path.exists(cmd.gitdir):
-cdup = read_pipe("git rev-parse --show-cdup").strip()
-if len(cdup) > 0:
-chdir(cdup);
-
-if not isValidGitDir(cmd.gitdir):
-if isValidGitDir(cmd.gitdir + "/.git"):
-cmd.gitdir += "/.git"
-else:
+if cmd.gitdir:
+os.environ["GIT_DIR"] = cmd.gitdir
+
+# did we get a valid git dir on the command line or via $GIT_DIR?
+if not gitdir():
die("fatal: cannot locate git repository at %s" % cmd.gitdir)
-os.environ["GIT_DIR"] = cmd.gitdir
+else:
+# already in a git directory?
+if not gitdir():
+die("fatal: not in a valid git repository")
+
+cdup = read_pipe("git rev-parse --show-cdup").strip()
+if len(cdup) > 0:
+chdir(cdup);
+
+# ensure subshells spawned in the p4 repo directory
+# get the correct GIT_DIR
+os.environ["GIT_DIR"] = os.path.abspath(gitdir())
if not cmd.run(args):
parser.print_help()
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 0730f18..093e9bd 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -257,6 +257,26 @@ test_expect_success 'submit from detached head' '
)
'
+test_expect_success 'submit from worktree' '
+ test_when_finished cleanup_git &&
+ git p4 clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git worktree add ../worktree-test
+ ) &&
+ (
+ cd "$git/../worktree-test" &&
+ test_commit "worktree-commit" &&
+ git config git-p4.skipSubmitEdit true &&
+ git p4 submit
+ ) &&
+ (
+ cd "$cli" &&
+ p4 sync &&
+ test_path_is_file worktree-commit.t
+ )
+'
+
test_expect_success 'kill p4d' '
kill_p4d
'
--
2.8.2.703.g78b384c.dirty
[PATCH] branch: mark a file-local symbol with static
Signed-off-by: Ramsay Jones
---
Hi Karthik,
If you need to re-roll your 'kn/ref-filter-branch-list' branch, could
you please squash this into the relevant patch (commit 715a4826ab,
"branch: use ref-filter printing APIs", 07-12-2016).
Thanks!
ATB,
Ramsay Jones
builtin/branch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2c9aa2b29..4386273ce 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -306,7 +306,7 @@ static int calc_maxwidth(struct ref_array *refs, int
remote_bonus)
return max;
}
-const char *quote_literal_for_format(const char *s)
+static const char *quote_literal_for_format(const char *s)
{
struct strbuf buf = STRBUF_INIT;
--
2.11.0
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
Duy Nguyen writes: > rebase and cherry-pick/revert are not exactly in the same situation. > When cherry-pick/revert in "continue/abort" mode, there's usually some > conflicted files and it's easy to notice. > > But an interactive rebase could stop at some commit with clean > worktree (the 'edit' command). Then I could even add some more commits > on top. I don't see how 'rebase --abort' can know my intention in this > case, whether I tried (with some new commits) and failed, and want to > revert/abort the whole thing, moving HEAD back to the original; or > whether I forgot I was in the middle of rebase and started to do > something else, and --abort needs to keep HEAD where it is. OK.
Re: [PATCH 3/4] doc: make the intent of sentence clearer
From: "Kristoffer Haugsbakk" Sent: Friday, December 09, 2016 3:51 PM By adding the word "just", which might have been accidentally omitted. Adding the word "just" makes it clear that the point is to *not* do an octopus merge simply because you *can* do it. In other words, you should have a reason for doing it beyond simply having two (seemingly) independent commits that you need to merge into another branch, since it's not always the best approach. The previous sentence made it look more like it was trying to say that you shouldn't do an octopus merge *because* you can do an octopus merge. Although this interpretation doesn't make sense and the rest of the paragraph makes the intended meaning clear, this adjustment should make the intent of the sentence more immediately clear to the reader. Signed-off-by: Kristoffer Haugsbakk --- Documentation/gitcore-tutorial.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt index 72ed90ca3..72ca9c1ef 100644 --- a/Documentation/gitcore-tutorial.txt +++ b/Documentation/gitcore-tutorial.txt @@ -1635,7 +1635,7 @@ $ git show-branch ++* [master~2] Pretty-print messages. -Note that you should not do Octopus because you can. An octopus +Note that you should not do Octopus just because you can. An octopus is a valid thing to do and often makes it easier to view the commit history if you are merging more than two independent changes at the same time. However, if you have merge conflicts -- 2.11.0 Looks like a reasonable emphasis to me -- Philip (UK)
Re: [PATCH 2/4] doc: add verb in front of command to run
From: "Kristoffer Haugsbakk"
Sent: Friday, December 09, 2016 3:51 PM
Instead of using the command 'git clone' as a verb, use "run" as the
verb indicating the action of executing the command 'git clone'.
I would expect 'cloning' as the commonly in use verb here, with the command
then quoted.
Signed-off-by: Kristoffer Haugsbakk
---
Documentation/gitcore-tutorial.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitcore-tutorial.txt
b/Documentation/gitcore-tutorial.txt
index 6c434aff3..72ed90ca3 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -1478,7 +1478,7 @@ You can repack this private repository whenever you
feel like.
A recommended work cycle for a "subsystem maintainer" who works
on that project and has an own "public repository" goes like this:
-1. Prepare your work repository, by 'git clone' the public
+1. Prepare your work repository, by running 'git clone' on the public
Perhaps ?
... by cloning ('git clone ') the public
The full command is shown using the same terminology as the following line.
repository of the "project lead". The URL used for the
initial cloning is stored in the remote.origin.url
configuration variable.
--
2.11.0
--
Philip
Re: [PATCH 1/4] doc: add articles (grammar)
From: "Kristoffer Haugsbakk" Sent: Friday, December 09, 2016 3:51 PM Add definite and indefinite articles in three places where they were missing. - Use "the" in front of a directory name - Use "the" in front of "style of cooperation" - Use an indefinite article in front of "CVS background" Signed-off-by: Kristoffer Haugsbakk --- Documentation/gitcore-tutorial.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt index 4546fa0d7..6c434aff3 100644 --- a/Documentation/gitcore-tutorial.txt +++ b/Documentation/gitcore-tutorial.txt @@ -1368,7 +1368,7 @@ $ git repack will do it for you. If you followed the tutorial examples, you would have accumulated about 17 objects in `.git/objects/??/` directories by now. 'git repack' tells you how many objects it -packed, and stores the packed file in `.git/objects/pack` +packed, and stores the packed file in the `.git/objects/pack` directory. [NOTE] @@ -1543,9 +1543,9 @@ like this: Working with Others, Shared Repository Style -If you are coming from CVS background, the style of cooperation +If you are coming from a CVS background, the style of cooperation suggested in the previous section may be new to you. You do not -have to worry. Git supports "shared public repository" style of +have to worry. Git supports the "shared public repository" style of cooperation you are probably more familiar with as well. See linkgit:gitcvs-migration[7] for the details. -- 2.11.0 This looks good to me. -- Philip (UK)
Re: [PATCH v2 4/5] Make sequencer abort safer
On 12/10/2016 09:04 PM, Jeff King wrote:
> On Sat, Dec 10, 2016 at 08:56:26PM +0100, Christian Couder wrote:
>
>>> +static int rollback_is_safe(void)
>>> +{
>>> + struct strbuf sb = STRBUF_INIT;
>>> + struct object_id expected_head, actual_head;
>>> +
>>> + if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
>>> + strbuf_trim(&sb);
>>> + if (get_oid_hex(sb.buf, &expected_head)) {
>>> + strbuf_release(&sb);
>>> + die(_("could not parse %s"),
>>> git_path_abort_safety_file());
>>> + }
>>> + strbuf_release(&sb);
>>> + }
>>
>> Maybe the following is a bit simpler:
>>
>>if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
>>int res;
>>strbuf_trim(&sb);
>>res = get_oid_hex(sb.buf, &expected_head);
>>strbuf_release(&sb);
>>if (res)
>>die(_("could not parse %s"),
>> git_path_abort_safety_file());
>>}
>
> Is there any point in calling strbuf_release() if we're about to die
> anyway? I could see it if it were "return error()", but it's normal in
> our code base for die() to be abrupt.
The point is that someone "libifies" the function some day; then "die()"
becomes "return error()" almost automatically. Chances are high that the
resulting memory leak is forgotten. That's one of the reasons why I like
being strict about memory leaks.
However, I cannot tell if mine or Christian's variant is really
"simpler" (with whatever measure) and I also don't care much.
~Stephan
Re: [PATCH v2 4/5] Make sequencer abort safer
On Sat, Dec 10, 2016 at 08:56:26PM +0100, Christian Couder wrote:
> > +static int rollback_is_safe(void)
> > +{
> > + struct strbuf sb = STRBUF_INIT;
> > + struct object_id expected_head, actual_head;
> > +
> > + if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
> > + strbuf_trim(&sb);
> > + if (get_oid_hex(sb.buf, &expected_head)) {
> > + strbuf_release(&sb);
> > + die(_("could not parse %s"),
> > git_path_abort_safety_file());
> > + }
> > + strbuf_release(&sb);
> > + }
>
> Maybe the following is a bit simpler:
>
>if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
>int res;
>strbuf_trim(&sb);
>res = get_oid_hex(sb.buf, &expected_head);
>strbuf_release(&sb);
>if (res)
>die(_("could not parse %s"), git_path_abort_safety_file());
>}
Is there any point in calling strbuf_release() if we're about to die
anyway? I could see it if it were "return error()", but it's normal in
our code base for die() to be abrupt.
-Peff
Re: [PATCH v2 4/5] Make sequencer abort safer
On Fri, Dec 9, 2016 at 8:01 PM, Stephan Beyer wrote:
[...]
> +static int rollback_is_safe(void)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + struct object_id expected_head, actual_head;
> +
> + if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
> + strbuf_trim(&sb);
> + if (get_oid_hex(sb.buf, &expected_head)) {
> + strbuf_release(&sb);
> + die(_("could not parse %s"),
> git_path_abort_safety_file());
> + }
> + strbuf_release(&sb);
> + }
Maybe the following is a bit simpler:
if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
int res;
strbuf_trim(&sb);
res = get_oid_hex(sb.buf, &expected_head);
strbuf_release(&sb);
if (res)
die(_("could not parse %s"), git_path_abort_safety_file());
}
Thanks,
Christian.
Re: Resend: Gitk: memory consumption improvements
Am 09.12.2016 um 18:57 schrieb Stefan Beller: > On Fri, Dec 9, 2016 at 3:51 AM, Markus Hitter wrote: >> >> It's a month now since I sent three patches to this list for reducing memory >> consumption of Gitk considerably: >> >> https://public-inbox.org/git/de7cd593-0c10-4e93-1681-7e123504f...@jump-ing.de/ >> https://public-inbox.org/git/e09a5309-351d-d246-d272-f527f50ad...@jump-ing.de/ >> https://public-inbox.org/git/8e1c5923-d2a6-bc77-97ab-3f154b41d...@jump-ing.de/ >> https://public-inbox.org/git/2cb7f76f-0004-a5b6-79f1-9bb4f979c...@jump-ing.de/ >> >> Everybody cheered, but apparently nobody picked these patches up and applied >> them to either the Git or Gitk repository. They don't appear in the regular >> "what's cooking" post either. > > The "What's cooking" email is done by Junio (the Git maintainer) > describing the development of Git, whereas gitk is maintained by Paul > if I understand correctly. TBH, there might be no Gitk maintenance at all, because the last commit in either repository dates February 2016. It's a bit hard to believe there was not a single contribution in 10 months. > I'd love to see those patches in use here (via a regular upstream update). > > So I guess the way to go for you is to keep bugging Paul for an ack? Like: Hey hey Paul! Come and get the goal! Fetch the patch and get applied to our all's delight! or like Macke- Macke- Mackerras! Come here and join with us! It's just a simple 'git am' and you are the man! Another encouraging poem? Markus -- - - - - - - - - - - - - - - - - - - - Dipl. Ing. (FH) Markus Hitter http://www.jump-ing.de/
Re: [BUG] Colon in remote urls
Hi Klaus, On Sat, 10 Dec 2016, Klaus Ethgen wrote: > -BEGIN PGP SIGNED MESSAGE- > Hash: SHA512 > > Am Fr den 9. Dez 2016 um 22:32 schrieb Johannes Sixt: > > There are too many systems out there that use a backslash in path names. I > > don't think it is wise to use it also as the quoting character. > > Well, the minority, I believe. And only the minority where the command > line git is used anywhere. Please provide evidence for such bold assumptions. Ciao, Johannes
Re: [PATCH v2 12/16] pathspec: create parse_long_magic function
Stefan Beller writes: > Jonathan Nieder mentioned off list that he prefers to see that > series rerolled without mutexes if possible. That is possible by > creating the questions "struct attr_check" before preloading the > index and then using the read only questions in the threaded code, > to obtain answers fast; also no need for a mutex. I do not see how it would work without further splitting the attr_stack. I think you made it per check[], but you would further split it per before losing the mutex, no?
Re: Any interest in 'git merge --continue' as a command
Jeff King writes: > No, I think your reasoning makes sense. But I also think we've already > choosen to have "--continue" mean "conclude the current, and continue if > there is anything left" in other contexts (e.g., a single-item > cherry-pick). It's more vague, but I think it keeps the user's mental > model simpler if we provide a standard set of options for multi-step > commands (e.g., always "--continue/--abort/--skip", though there are > some like merge that omit "--skip" if it does not make sense). Yup. I know you know me well enough to know that I didn't mean to say "oh this one needs to be called differently" ;-) I just felt that "--continue" in that context did not sit well.
Re: [REGRESSION 2.10.2] problematic "empty auth" changes
On Sat, Dec 10, 2016 at 03:52:39PM +0100, Johannes Schindelin wrote: > One of my colleagues offered a legitimate concern: it potentially adds > another round-trip. > > Do you happen to know whether regular HTTPS negotiation will have an extra > round-trip if Kerberos is attempted, but we have to fall back to > interactively prompt for (or use stored) credentials? With Kerberos (using tickets), you have 7 request/response pairs, and an additional round trip for the 100 Continue if your push is larger than http.postBuffer. You only have 6 for Basic using Kerberos. However, libcurl is generally going to be able to figure out whether your Kerberos credentials can be used, so when it falls back to Basic, it does so because it knows you have nothing to use with Negotiate (e.g. you have no ticket), and therefore it doesn't even try. I suppose if I tried to push to a server that offered Negotiate and Basic, but didn't accept my Kerberos credentials, it might fall back in such a way, though. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [REGRESSION 2.10.2] problematic "empty auth" changes
Hi Brian, On Fri, 9 Dec 2016, brian m. carlson wrote: > On Thu, Dec 08, 2016 at 04:12:32PM -0500, David Turner wrote: > > I know of no reason that shouldn't work. Indeed, it's what we use do > > internally. So far, nobody has reported problems. That said, we have > > exactly three sets of git servers that most users talk to (two > > different internal; and occasionally github.com for external stuff). > > So our coverage is not very broad. > > > > If you're going to do it, tho, don't just do it for Windows users -- > > do it for everyone. Plenty of Unix clients connect to Windows-based > > auth systems. > > Let me echo this. This would make Kerberos (and probably other forms of > SPNEGO) work out of the box, which would reduce a lot of confusion that > people have. > > I can confirm enabling http.emptyAuth works properly with Kerberos, > including with fallback to Basic, so I see no reason why we shouldn't do > it. One of my colleagues offered a legitimate concern: it potentially adds another round-trip. Do you happen to know whether regular HTTPS negotiation will have an extra round-trip if Kerberos is attempted, but we have to fall back to interactively prompt for (or use stored) credentials? Ciao, Johannes
Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
On 12/08/2016 04:38 AM, vi0...@gmail.com wrote: Third review: missing && in test fixed. Shall something more be done about this or just wait until the patch gets reviewed and integrated?
Re: [PATCHv7 4/6] worktree: have a function to check if worktrees are in use
On Sat, Dec 10, 2016 at 1:49 AM, Stefan Beller wrote:
> On Fri, Dec 9, 2016 at 4:00 AM, Duy Nguyen wrote:
>
>> int submodule_uses_worktrees(const char *path)
>> {
>> struct strbuf path = STRBUF_INIT;
>> DIR *dir;
>> struct dirent *d;
>> int ret = 0;
>>
>> strbuf_addf(&path, "%s/worktrees", path);
>> dir = opendir(path.buf);
>> strbuf_release(&path);
>>
>> if (!dir)
>> return 0;
>
> The submodule may be one of the linked worktrees, which would be
> caught if we use the code as I sent it out?
I think I simplified it too much, there should still be a
git_common_dir_noenv() to retrieve the correct common dir in the
submodule from gitdir. Then, if the repo in question has any linked
worktrees, you probably can't handle it. It does not matter if the
submodule gitdir in question is a linked or main worktree.
> If this is one of the linked worktrees, we'd rather check if a file
> "commondir" or "gitdir" exists?
Well, in theory yes. But we're apparently not ready for that. If you
check the files exist, but the files are not valid, then it's still
not considered a worktree. Which is not much different from what we do
here (if the directory exists, assuming it's a worktree). It should
catch all the valid worktrees. But yes it could give some false
positive. Since we're just using this function to stop the aborbing
git dir operation, i think this is acceptable.
(It goes even trickier, even get_worktrees() won't detect if the
linked worktree is already dead, .e.g. deleted manually, I believe.
Those have to be cleaned up by 'git worktree prune' which does even
more checks before it declare "this directory is garbage").
> I ask that because I would not know how to relocate such a linked
> worktree gitdir?
--
Duy
Re: Feature request: read git config from parent directory
On Friday, 9 December 2016 19:38:05 CET Duy Nguyen wrote: > >> Sounds like the same problem I have (and the reason I came up with > >> conditional include [1]). Would that work for you (check out the > >> example part in that patch)? > > > > If I understand correcly, I would need to set up config include in each > > git > > repository. This is as much work as setting up user.email in the same > > place. > > Well, no. You set this up in ~/.gitconfig. If you need to add some > settings in /abc/def/.gitconfig and expect repositories in this path > to reach it via the parent chain, then you could write something like > > [include "gitdir:/abc/def/"] > file = your-config-file > > in ~/.gitconfig and achieve the same effect, because all repos will > read ~/.gitconfig, and if it finds out the repo's location is inside > /abc/def, your-config-file will be loaded. It could contain email > settings or whatever. > > So, instead of spreading .gitconfig files around and relying on > parent-chain to reach them, you write a few filter rules in > ~/.gitconfig to tell all the repos what to load. oh... yes, that would solve my problem and have no impact on other user who don't need this feature. I do hope that the improvement you proposed will be merged. Thanks for the explanation. All the best -- https://github.com/dod38fr/ -o- http://search.cpan.org/~ddumont/ http://ddumont.wordpress.com/ -o- irc: dod at irc.debian.org
Re: [PATCH v2 0/4] road to reentrant real_path
On Sat, Dec 10, 2016 at 2:42 AM, Brandon Williams wrote: > On 12/09, Duy Nguyen wrote: >> On Fri, Dec 9, 2016 at 6:58 AM, Brandon Williams wrote: >> > diff --git a/setup.c b/setup.c >> > index fe572b8..0d9fdd0 100644 >> > --- a/setup.c >> > +++ b/setup.c >> > @@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const >> > char *gitdir) >> > if (!is_absolute_path(data.buf)) >> > strbuf_addf(&path, "%s/", gitdir); >> > strbuf_addbuf(&path, &data); >> > - strbuf_addstr(sb, real_path(path.buf)); >> > + strbuf_realpath(sb, path.buf, 1); >> >> This is not the same because of this hunk in strbuf_realpath() > > Then perhaps I shouldn't make this change (and just leave it as is) > since the way real_path_internal/strbuf_realpath is written requires > that the strbuf being used for the resolved path only contains the > resolved path (see the lstat(resolved->buf &st) call). Sidenote it > looks like strbuf_getcwd() also does a reset, though more subtlety, > since it just passes its buffer to getcwd(). Yeah that's ok too (I did not see this subtlety when I suggested the change). -- Duy
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
On Sat, Dec 10, 2016 at 2:24 AM, Stephan Beyer wrote: > Hi Junio, > > On 12/09/2016 07:07 PM, Junio C Hamano wrote: >> Duy Nguyen writes: >>> Having the same operation with different names only increases git >>> reputation of bad/inconsistent UI. Either forget is renamed to quit, >>> or vice versa. I prefer forget, but the decision is yours and the >>> community's. So I'm sending two patches to rename in either direction. >>> You can pick one. >> >> I actually was advocating to remove both by making --abort saner. >> With an updated --abort that behaves saner, is "rebase --forget" >> still necessary? > > A quick change in t3407 of the "rebase --forget" test to use "rebase > --abort" failed. That's because it checks the use-case of > forgetting/aborting without changing the HEAD. So --abort makes a > rollback, --forget just keeps the current head. I am not sure if that > tested use-case is a real use-case though. It is. I wanted something like this for years but "rm -rf /path/to/.git/rebase*" was not as bad when there were no linked worktrees. rebase and cherry-pick/revert are not exactly in the same situation. When cherry-pick/revert in "continue/abort" mode, there's usually some conflicted files and it's easy to notice. But an interactive rebase could stop at some commit with clean worktree (the 'edit' command). Then I could even add some more commits on top. I don't see how 'rebase --abort' can know my intention in this case, whether I tried (with some new commits) and failed, and want to revert/abort the whole thing, moving HEAD back to the original; or whether I forgot I was in the middle of rebase and started to do something else, and --abort needs to keep HEAD where it is. -- Duy
Re: Any interest in 'git merge --continue' as a command
On Sat, Dec 10, 2016 at 1:00 AM, Jeff King wrote: > On Sat, Dec 10, 2016 at 09:49:13PM +1300, Chris Packham wrote: > >> > There is nothing to "continue" in a stopped merge where Git asked >> > for help from the user, and because of that, I view the final "git >> > commit" as "concluding the merge", not "continuing". "continue" >> > makes quite a lot of sense with rebase and cherry-pick A..B that >> > stopped; it concludes the current step and let it continue to >> > process the remainder. So from that point of view, it somewhat >> > feels strange to call it "merge --continue", but it probably is just >> > me. >> >> Yeah I did think that --continue wasn't quite the right word. git >> merge --conclude would probably be the most accurate. > > I'd be against giving it a subtly-different name. It's just going to > frustrate people who cannot remember when to use "--conclude" and when > it is "--continue". The strength of the proposal, IMHO, is that it > abstracts the idea of "go on to the next thing or finish" across many > commands. > > -Peff Agreed. I think "continue" makes sense as the command had to "stop" the merge so you could give input, and then you tell git to "continue" which also happens to mean "finish the merge" and yes it may not be 100% accurate, but the point of adding "git merge --continue" is that it simplifies the mental model between rebase, cherry-pick, and merge, all of which stop and ask the user to resolve a conflict before "continue"ing and finalizing that resolution. Thanks, Jake
Re: [BUG] Colon in remote urls
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Am Sa den 10. Dez 2016 um 11:26 schrieb Jeff King: > On Sat, Dec 10, 2016 at 10:41:33AM +0100, Klaus Ethgen wrote: > > > Am Sa den 10. Dez 2016 um 9:26 schrieb Jeff King: > > > Yeah, I picked it arbitrarily as the common quoting character, but I > > > agree it probably makes backwards compatibility (and general usability > > > when you have to double-backslash each instance) pretty gross on > > > Windows. > > > > Well, I don't know of many people using the original git on windows. > > Most of them using some graphical third-party tools. > > I laid out options for addressing the problem elsewhere, but I want to > make clear that this line of reasoning is not likely to get any traction > here. Git for Windows is a non-trivial part of the user base, and we do > care about avoiding regressions there. I value that. I just wanted to point out what my observations are and too I don't want to see that windows stuff (what is the minority in this case) will get more attention or more priority as stuff on linux (which pretty much is the majority, I believe). Don't get me wrong, I see them both as important. And even on windows there are pathes with colon in it. (read c:\..., d:\...) Regards Klaus - -- Klaus Ethgen http://www.ethgen.ch/ pub 4096R/4E20AF1C 2011-05-16Klaus Ethgen Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C -BEGIN PGP SIGNATURE- Comment: Charset: ISO-8859-1 iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhL3ksACgkQpnwKsYAZ 9qznAgv+IPrgt2yvFE9m/YpZv7viSlV9SsIyr6DjvK1C06N9IZes69z4yywTSjVf L5x4ecWEg+k4N7ET+qNp1iaFS70juNvWg0hysbteczxElNGOerR8pEWXxKA+8xpN tcY55ZcbBgihPDlQ+ztHAGlngD7E3wwMbzSC5kGCPYYe7Kw9mhLrXzX/MN5V2H/k 60XXsTkW05kUg01ofFATNppFLJHDTFJhEHiZmytJ5se5fTAUXM0gwFkCYEcPh+72 d6GsodVNRW++i9ojAEqglPbAaEAAFg1MOULa3KN2xsIYhvyZ8P4hNc2DzPcAsdtr 8ngoJb80XQ+gLzwJ80RjUnUNFLE+BFUzLrCRxgYyjk3kD7rewLUFHJkte05WBTRv t21V/AudDC8i8z7XnQtAFpV8QHjTv00nMqV9e2iaTqmd7QJ8bPvDQmSu86/x3MMM /2aFUcPmRu1Pp+XGm8KUDOF+kba8IJAi7Zs0UCNdyj6domjZBsE9N3pKkcbxsNBJ 0+5Ln4Vi =twtR -END PGP SIGNATURE-
Re: [BUG] Colon in remote urls
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Hi, Am Sa den 10. Dez 2016 um 11:24 schrieb Jeff King: > > A colon a perfectly allowed character in POSIX filesystems. > > Sure, it's allowed, but it will cause problems due to other syntactic > conventions. Try putting "/usr/path:with:colons" into your $PATH > variable, for instance. True. > Try rsyncing "xxx:yyy.git" somewhere. Only a problem when the part before the colon has no directory limiter (/) in it. And even then there is ways to work around that limitation. And in this case, it is pretty good documented. As I told, I never heared of such limitation in git commands. > Git does have heuristics for figuring out the difference between > "host:repo.git" as an SSH remote versus a local path, but they're not > foolproof. Well, that is the reason why I first tried to solve it via file://... Note, I have no problem with it, if that char has to be qouted somehow; if it is clearly documented. But also then, the handling should be consistent. In git (in this version) it is not. Pull works without problems but push dosn't. > > Moreover, it was no problem before and was introduced as a problem just > > in that version. Even more, a pull (and so a clone I believe) of such a > > path is absolutely ok. Just the push fails. > > Sort of. This has always been a problem with the variables Junio > mentioned. The change in v2.11 is that the alternates subsystem is being > used in some cases where it wasn't before, which is surfacing this > limitation in more places. - -v please. I didn't get it with that alternate stuff in push. A link to some man page is ok too. > > > directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is > > > added without splitting to the list of alternate object stores, and > > > the quarantine codepath can export that instead. > > > > I didn't get it, why is there a need to split? I mean, it is not > > possible to push to two locations at the same time, so why is there > > splitting at all? > > Because the new quarantine feature[1] is built on top of the existing > alternates mechanism, which can have several sources. I'll clone the repo and read about, thanks for the pointer. I even have to find out about that alternates mechanism and what it has to do with push but not with pull. Regards Klaus - -- Klaus Ethgen http://www.ethgen.ch/ pub 4096R/4E20AF1C 2011-05-16Klaus Ethgen Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C -BEGIN PGP SIGNATURE- Comment: Charset: ISO-8859-1 iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhL3PIACgkQpnwKsYAZ 9qwVXAv/VQPtAfete9ZwVUqjdiALVv6n6Cyy+AyTNyLsj56/83ibl5OIBJyr1qDb e1QueLKEH6qyln+rC5vgejjq4AHk5aPhAx1IpJaaKJKh298c+IImIMxy+84m8xt5 x39u8Kwuz3oGQNshlTed3/qVUR/zzLzPhbFuvHKk3wXDcIJHjDBmBJ4CWyvmyNOh OfOnXaqu4ohNJpwlCNsJvojjstdcpl6Uj7UM5BIdmw1JFuZClw0ljWLHDqC1YIma 2QbcJ7eY29E+uR6sJKbXuWLgVDE+RrhbBn/GVBATxP5giBLa2z7+gLSMwxcZjTmv 2gd5nhjBL0aIWrr7IIsgcW9I0P/PTazMGSSBsgupjXqA+/pEBL1ePFoK930F/Hdx Bqh28jT5OYDoaIQHHDm3z6eZzqysdT2tc+uvBBa+g9NA/D6RzEw1qeRmNvvd4UeS r8rENu7+nLibUE4JjQqDWKwF6FsZkwVH5xXZZ/VQoZHfFQep73ZnQGz3PcfKXGkl 5BMLh24Y =FX+B -END PGP SIGNATURE-
Re: [BUG] Colon in remote urls
On Sat, Dec 10, 2016 at 10:41:33AM +0100, Klaus Ethgen wrote: > Am Sa den 10. Dez 2016 um 9:26 schrieb Jeff King: > > Yeah, I picked it arbitrarily as the common quoting character, but I > > agree it probably makes backwards compatibility (and general usability > > when you have to double-backslash each instance) pretty gross on > > Windows. > > Well, I don't know of many people using the original git on windows. > Most of them using some graphical third-party tools. I laid out options for addressing the problem elsewhere, but I want to make clear that this line of reasoning is not likely to get any traction here. Git for Windows is a non-trivial part of the user base, and we do care about avoiding regressions there. -Peff
Re: [BUG] Colon in remote urls
On Sat, Dec 10, 2016 at 10:29:28AM +0100, Klaus Ethgen wrote: > > I think we long time ago in 2005 have declared that a colon in a > > directory name would not work for Git repositories because of things > > like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I > > do not think we terribly mind that direction. > > That is the first I hear and I really wonder about. > > A colon a perfectly allowed character in POSIX filesystems. Sure, it's allowed, but it will cause problems due to other syntactic conventions. Try putting "/usr/path:with:colons" into your $PATH variable, for instance. Try rsyncing "xxx:yyy.git" somewhere. Git does have heuristics for figuring out the difference between "host:repo.git" as an SSH remote versus a local path, but they're not foolproof. > Moreover, it was no problem before and was introduced as a problem just > in that version. Even more, a pull (and so a clone I believe) of such a > path is absolutely ok. Just the push fails. Sort of. This has always been a problem with the variables Junio mentioned. The change in v2.11 is that the alternates subsystem is being used in some cases where it wasn't before, which is surfacing this limitation in more places. > > directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is > > added without splitting to the list of alternate object stores, and > > the quarantine codepath can export that instead. > > I didn't get it, why is there a need to split? I mean, it is not > possible to push to two locations at the same time, so why is there > splitting at all? Because the new quarantine feature[1] is built on top of the existing alternates mechanism, which can have several sources. I do think we should address this as a regression, but I think repo names with colons are always going to suffer from some corner cases. -Peff [1] See 25ab004c5 and the commits leading up to it for more discussion of what the new feature is, if you're curious.
Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
A Sex, 09-12-2016 às 14:23 -0800, Junio C Hamano escreveu:
> > This is exactly the same issue I fixed for rebase -i recently.
>
> Yes, but the patch we see here punts "core.commentChar is not a
> single-byte single-letter--panic!" case differently. I think you
> did "just take the first one" in "rebase -i", which I think is more
> in line with the rest of the system, and this addition to Git.pm
> should do the same, I think.
I hope the changes below are in line with the rest of the system. If
so, I will send a new re-roll with them.
I wonder why this is important when Git errors out when
core.commentChar is set to more than 1 characters or 0 characters. Is
it just to be consistent with "rebase -i" changes introduced
by Johannes Schindelin?
I am not sure what does "if (length($comment_line_char) != 1)" check.
Whether it checks single-byte or single-letter or both...
-- >8 --
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3a6d846..4e0ab5a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1072,7 +1072,7 @@ sub edit_hunk_manually {
print $fh @$oldtext;
my $is_reverse = $patch_mode_flavour{IS_REVERSE};
my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') :
('+', '-');
- my $comment_line_char = Git::config("core.commentchar") ||
'#';
+ my $comment_line_char = Git::get_comment_line_char;
print $fh Git::comment_lines sprintf(__ <
Re: [BUG] Colon in remote urls
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Am Sa den 10. Dez 2016 um 9:26 schrieb Jeff King: > Yeah, I picked it arbitrarily as the common quoting character, but I > agree it probably makes backwards compatibility (and general usability > when you have to double-backslash each instance) pretty gross on > Windows. Well, I don't know of many people using the original git on windows. Most of them using some graphical third-party tools. The main git suite is most often used on linux where a colon is a valid character For example using /mnt/c: as mount path for windows file systems or /bla/foo/dated_repository_2016-12-10_12:00.git for dated and timed repositories. My btrfs snapshot dir looks like: ~snapshot> l -gGhN [...] drwxr-x--x 1 296 2016-07-30T13:55 daily_2016-12-10_00:00:01.270213478 drwxr-x--x 1 296 2016-07-30T13:55 hourly_2016-12-10_05:00:01.372037552 [...] Compared to the backslash, although it is a perfect legal character in POSIX file systems, I do not know any use of it. Regards Klaus - -- Klaus Ethgen http://www.ethgen.ch/ pub 4096R/4E20AF1C 2011-05-16Klaus Ethgen Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C -BEGIN PGP SIGNATURE- Comment: Charset: ISO-8859-1 iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhLzc0ACgkQpnwKsYAZ 9qy1jQv/Wcafo8nJuy/dNIpxN5tNaLEENrY6a2dkv379F2miEJYROlWO6UzG86hY 0WIZAm5BKK6SpPVztTMcs2GHPF0iCB4V4RyQFdFa73OhaAgHOJRdy50eaGSz6vt6 lDZkJZsG0FoXcT6Fapdl5xZeoNDXjPcYH/7yFQ7VjMD5HTpLDIs8E5Mb8V1jwehV JKzQd136vksS2qB96jElAYonXFwImvYfTplH3nELJh/kKRJOT8Mzgj/+X7vxnQcC NISiLysSxqPm5d9yDsfN1eofMNGn2zgJZStOP6jNV2yqldMgN0fJX4Mt449GpBO8 OSYjN828QsDYXCWdTCKxbLCxjfNxfvQgHHR7ugSlf9xPrro3MjQjg2cMhZ/fCzCm XcC4X+Iyec2F0wHSQiXqlb7wiOXa1Oup6zmTRe/G5HkhlCap/+R2nOCfkqEEwhkB moYTqfETqqTJUJiiYVM/U8LBFWGnBBCGWgRPzyNdFna+WnvD93s9JPeg7q9qFm6x 8flMJBm8 =M5IW -END PGP SIGNATURE-
Re: [BUG] Colon in remote urls
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Am Fr den 9. Dez 2016 um 22:32 schrieb Johannes Sixt: > There are too many systems out there that use a backslash in path names. I > don't think it is wise to use it also as the quoting character. Well, the minority, I believe. And only the minority where the command line git is used anywhere. But for the majority of OS, where the command line git is in use (instead of graphically third party git tools) is perfect known for backslash as escaping character. However, don't forget that a backslash could also be part of the file name. Regads Klaus - -- Klaus Ethgen http://www.ethgen.ch/ pub 4096R/4E20AF1C 2011-05-16Klaus Ethgen Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C -BEGIN PGP SIGNATURE- Comment: Charset: ISO-8859-1 iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhLy64ACgkQpnwKsYAZ 9qzYfQwAmVIR+bVOvOcZ+yu7HddC5mwo7st6w+vPcdLKpFWcHIhsG//cHq6he+mm /Dmfhnc4Yp+dSy7Z99p9DV67hAj0Zxj2koxBo4eCdwWhnKrphCHSST8j2IxIg/0h Y1axQEBc5hV9nImTYmOks0pa5c9wKkZS36aTjP63PKgIv46A8RDY/QXAm2uO4kjj gfBEiDVkQA99QlvpP1qbuRCK3QUmfqxrP9ldiAhmuDBbNv2smiBxhoN3E6NgKTJG fM6WjaZPUqpDUiky5gO0xfOpf6s2c/GMTEO1I5aWom6VKtrOoYUyMlyeMiqALWj7 KfN7TJfDqp3THo0AkLXuukrNMv9gdfgiAimGqYgSfFM9P60aPjGeC7nBt875PSae jdVjIGyl0tHZzSIbBoSecQqx8h65eZaHLaS1uNf704m+EwIs1X2daI7Re3DcAyzL EWJxtZyZbG/GooCdA9deywlEAtGNOdIg+p+YkThsmEGiaOir/fAMyviSP/jONTCq OC5oHOcK =x9y4 -END PGP SIGNATURE-
Re: [BUG] Colon in remote urls
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 Hello, Am Fr den 9. Dez 2016 um 20:07 schrieb Junio C Hamano: > Jeff King writes: > > (One other option is to just declare that the quarantine feature doesn't > > work with colons in the pathname, but stop turning it on by default. I'm > > not sure I like that, though). > > I think we long time ago in 2005 have declared that a colon in a > directory name would not work for Git repositories because of things > like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I > do not think we terribly mind that direction. That is the first I hear and I really wonder about. A colon a perfectly allowed character in POSIX filesystems. Moreover, it was no problem before and was introduced as a problem just in that version. Even more, a pull (and so a clone I believe) of such a path is absolutely ok. Just the push fails. > > Here's a rough idea of what the quoting solution could look like. It > > should make your case work, but I'm not sure what we want to do about > > backwards-compatibility, if anything. > > Yes, obviously it robs from those with backslash in their pathnames > to please those with colons; we've never catered to the latter, so I > am not sure if the trade-off is worth it. As I quote above, a colon is perfect common in POSIX filesystems. A backslash is at least uncommon and always needed to quote as it, most often, has special meaning to os/shell. I cannot see why a tool (as git is) should decide what characters are "bad" and what are "good". If the filesystem beneath supports it... By the way, I didn't find anywhere in git documentation that there are "bad" chars around. > I can see how adding a new environment variable could work, though. > A naive way would be to add GIT_ALT_OBJ_DIRS that uses your quoting > when splitting its elements, give it precedence over the existing > one (or allow to use both and take union as the set of alternate > object stores) and make sure that the codepaths we use internally > uses the new variable. But if the quarantine codepath will stay to > be the only user of this mechanism (and I strongly suspect that will > be the case), the new environment could just be pointing at a single > directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is > added without splitting to the list of alternate object stores, and > the quarantine codepath can export that instead. I didn't get it, why is there a need to split? I mean, it is not possible to push to two locations at the same time, so why is there splitting at all? Regards Klaus - -- Klaus Ethgen http://www.ethgen.ch/ pub 4096R/4E20AF1C 2011-05-16Klaus Ethgen Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C -BEGIN PGP SIGNATURE- Comment: Charset: ISO-8859-1 iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhLyvIACgkQpnwKsYAZ 9qyBzgv9EzMEWrEgsTd1Z0gjpzpJlhpO8R2I7H4mGvEjlxoTXtUNwjvM+ojAYzaI F34IBRv9BCha+h7I6ccoaAsfmurz73lA1AKy1IFPrYAoxompYLomC9exY+8+ggdN G2uVbMTmiL+CxJGo0ItxmiQCcv7himVoyto70Dekc7se+panbzCq/vG4+Rz7pwRn sWnlKs0tQomi6QXbibo8992v4ECkAXzE2Xc/l5DvosSwNNPsqgdeeiNHEMDTbQq8 jqencfOruCfyMnQ0ejCaTWNbYY5SVUtfWikwta12jB06D1BgHTCRVKZCfhoHJx5+ Ffqj8uuiCJuZGQcopzJWiYU5X+SUHz7Ya+iA3VQOxNEKsGAZgq6QQqDcd0y9fyPt pzMAYo26GRioDiuQZzZ4CzA5eC0wWozv3oESsKLD5RsbWHV/9ODbpr7lHMW2TmUp H2vZhre1K/ZX2bODQByJoRNtDUqKvZmI6GsbXrvRAFKF4aCLByFIgcrZprmj++DH jlb25tjq =jOGb -END PGP SIGNATURE-
Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email
On Fri, Dec 09, 2016 at 12:34:49PM -0800, Stefan Beller wrote:
> My first perl contribution to Git. :)
Yes, I have some style suggestions below. :)
> Marked as RFC to gauge general interest before writing tests and
> documentation.
It's hard to evaluate without seeing an example of what you'd actually
want to put into a hook.
> diff --git a/git-send-email.perl b/git-send-email.perl
> index da81be40cb..d3ebf666c3 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -815,6 +815,15 @@ if (!$force) {
> . "Pass --force if you really want to send.\n";
> }
> }
> + my @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )
It's awkward to use a list here, when you just end up accessing
$hook[0]. You can form the list on the fly when you call system(). You
also probably are missing a trailing "/".
I'm not sure that $GIT_DIR is consistently set; you probably want to
look at "git rev-parse --git-dir" here. But I think we make a Git
repository object earlier, and you can just do:
my $hook = join('/', $repo->repo_path(), 'hooks/send-email');
Or you can just do string concatenation:
my $hook = $repo->repo_path() . '/hooks/send-email';
If I were writing repo_path myself, I'd probably allow:
my $hook = $repo->repo_path('hooks/send-email');
like we do with git_path() in the C code. It wouldn't be hard to add.
> + if( -x $hook[0] ) {
Funny whitespace. I think:
if (-x $hook)
would be more usual for us.
> + unless( system( @hook ) == 0 )
> + {
> + die "Refusing to send because the patch\n\t$f\n"
> + . "was refused by the send-email hook."
> + . "Pass --force if you really want to send.\n";
> + }
I like "unless" as a trailing one-liner conditional for edge cases,
like:
do_this($foo) unless $foo < 5;
but I think here it just makes things more Perl-ish than our code base
usually goes for. Probably:
if (system($hook, $f) != 0) {
die ...
}
would be more usual for us (in a more Perl-ish code base I'd probably
write "system(...) and die ...").
-Peff
Re: Any interest in 'git merge --continue' as a command
On Sat, Dec 10, 2016 at 09:49:13PM +1300, Chris Packham wrote: > > There is nothing to "continue" in a stopped merge where Git asked > > for help from the user, and because of that, I view the final "git > > commit" as "concluding the merge", not "continuing". "continue" > > makes quite a lot of sense with rebase and cherry-pick A..B that > > stopped; it concludes the current step and let it continue to > > process the remainder. So from that point of view, it somewhat > > feels strange to call it "merge --continue", but it probably is just > > me. > > Yeah I did think that --continue wasn't quite the right word. git > merge --conclude would probably be the most accurate. I'd be against giving it a subtly-different name. It's just going to frustrate people who cannot remember when to use "--conclude" and when it is "--continue". The strength of the proposal, IMHO, is that it abstracts the idea of "go on to the next thing or finish" across many commands. -Peff
Re: Any interest in 'git merge --continue' as a command
On Fri, Dec 09, 2016 at 11:16:52AM -0800, Junio C Hamano wrote: > > It seems like that would be in line with 35d2fffdb (Provide 'git merge > > --abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated > > goal was providing consistency with other multi-command operations. > > > > I assume it would _just_ run a vanilla "git commit", and not try to do > > any trickery with updating the index (which could be disastrous). > > If we were to have "merge --continue", I agree that it would be the > logical implementation. > > There is nothing to "continue" in a stopped merge where Git asked > for help from the user, and because of that, I view the final "git > commit" as "concluding the merge", not "continuing". "continue" > makes quite a lot of sense with rebase and cherry-pick A..B that > stopped; it concludes the current step and let it continue to > process the remainder. So from that point of view, it somewhat > feels strange to call it "merge --continue", but it probably is just > me. No, I think your reasoning makes sense. But I also think we've already choosen to have "--continue" mean "conclude the current, and continue if there is anything left" in other contexts (e.g., a single-item cherry-pick). It's more vague, but I think it keeps the user's mental model simpler if we provide a standard set of options for multi-step commands (e.g., always "--continue/--abort/--skip", though there are some like merge that omit "--skip" if it does not make sense). -Peff
Re: git add -p with new file
On Fri, Dec 09, 2016 at 01:43:24PM -0500, Ariel wrote: > > It's contrary to the rest of git-add for specifying pathspecs to > > actually make things _more_ inclusive rather than less. > > Is it? Because git add without -p is happy to add new files. I was just speaking there of whether the presence of the file on the command-line was relevant. In other words, "git add -u untracked-file" does not countermand the "-u" to say "also add this file". > > Historically "add -p" has been more like "add -u" in updating tracked > > files. > > But it doesn't have to be that way. You could make add -p identical to add > without options, except the -p prompts to review diffs first. The question is whether you would annoy people using "-p" if you started including untracked files by default. I agree because it's inherently an interactive process that we can be looser with backwards compatibility. Perhaps a config option would be the best path forward (even if we were to switch the default to "true", it leaves an escape hatch for people who do not like the new behavior). > > We have "-A" for "update everything _and_ new files". It doesn't > > seem unreasonable to me to have a variant of "-p" that is similar. > > That seems unnecessarily complex because -p asks about each file, so you > will never find new files added without realizing it. If you care about adding new files, wouldn't you just always use "-P" instead of "-p"? -Peff
Re: [BUG] Colon in remote urls
On Fri, Dec 09, 2016 at 11:07:25AM -0800, Junio C Hamano wrote: > Jeff King writes: > > > (One other option is to just declare that the quarantine feature doesn't > > work with colons in the pathname, but stop turning it on by default. I'm > > not sure I like that, though). > > I think we long time ago in 2005 have declared that a colon in a > directory name would not work for Git repositories because of things > like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I > do not think we terribly mind that direction. I don't mind declaring the feature incompatible. But I'm not sure whether I like not having it on by default, if only because it means we have two distinct code paths to care about. They're sufficiently different that I'd worry about a bug creeping into one and not the other. > > Here's a rough idea of what the quoting solution could look like. It > > should make your case work, but I'm not sure what we want to do about > > backwards-compatibility, if anything. > > Yes, obviously it robs from those with backslash in their pathnames > to please those with colons; we've never catered to the latter, so I > am not sure if the trade-off is worth it. > > I can see how adding a new environment variable could work, though. > A naive way would be to add GIT_ALT_OBJ_DIRS that uses your quoting > when splitting its elements, give it precedence over the existing > one (or allow to use both and take union as the set of alternate > object stores) and make sure that the codepaths we use internally > uses the new variable. Yeah, a new variable solves the backwards-compatibility issue, though if we continue to use backslash as an escape, then people on Windows will annoying _have_ to backslash-escape "C:\\" (worse, AIUI the conversion from "/unix/path" to "C:\unix\path" happens transparently via msys magic, and it would not know that we need to quote). I think the least-gross alternative would be to make newline the new delimiter. It's already the delimiter (and not quotable) in the objects/info/alternates file, and I have a lot less trouble telling people with newline in their filenames that they're Doing It Wrong. > But if the quarantine codepath will stay to > be the only user of this mechanism (and I strongly suspect that will > be the case), the new environment could just be pointing at a single > directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is > added without splitting to the list of alternate object stores, and > the quarantine codepath can export that instead. Yes, I also considered that approach. The big downside is that it does not help other users of GIT_ALTERNATE_OBJECT_DIRECTORIES. I doubt that matters much in practice, though. My other question is whether we care about compatibility between Git versions here. If receive-pack sets the variable, we can assume that index-pack will be run from the same version. But we also run hooks with the quarantine variables set. The nice thing about the existing scheme is that older versions of Git (or alternate implementations of Git) will just work, because it builds on a mechanism that has existed for ages. And that's the one thing that a quoting scheme has going for it: it only impacts the variable when there is something to be quoted. So if your repo path has a colon in it (or semi-colon on Windows) _and_ you are calling something like jgit from your hook, then you might see a failure. But either of those by itself is not a problem. Side note: It makes me wonder if all implementations even support GIT_ALTERNATE_OBJECT_DIRECTORIES. JGit seems to, looks like libgit2 does not. The most backwards-compatible thing is not enabling quarantine by default, and then there's no chance of regression, and you are responsible for making sure you have a compatible setup before turning the feature on. But like I said, I'd love to avoid that if we can. So here's the array of options, as I see it: 1. Disable quarantine by default; only turn it on if you don't have any of the funny corner cases. 2. Introduce an extra single-item environment variable that gets appended to the list of alternates, and side-steps quoting issues. 3. Introduce a new variable that can hold an alternate list, and either teach it reasonable quoting semantics, or give it a less-common separator. 4. Introduce quoting to the existing variable, but make the quoting character sufficiently obscure that nobody cares about the lack of backwards compatibility. I actually think (4) is reasonably elegant, except that the resulting quoting scheme would be vaguely insane to look at. E.g., if we pick newline as the quote character (not the separator), then you end up with: $ repo=foo:bar.git $ GIT_ALTERNATE_OBJECT_DIRECTORIES=$(echo $repo | perl -pe 's/:/\n$&/') $ echo "$GIT_ALTERNATE_OBJECT_DIRECTORIES" foo :bar.git It's pleasant for machines, but not for humans. So I dunno. Opinions on those 4 options are welcome.
Re: Any interest in 'git merge --continue' as a command
On Sat, Dec 10, 2016 at 8:16 AM, Junio C Hamano wrote: > Jeff King writes: > >>> They knew about git rebase --continue (and git am and git cherry-pick) >>> but they were unsure how to "continue" a merge (it didn't help that >>> the advice saying to use 'git commit' was scrolling off the top of the >>> terminal). I know that using 'git commit' has been the standard way to >>> complete a merge but given other commands have a --continue should >>> merge have it as well? >> >> It seems like that would be in line with 35d2fffdb (Provide 'git merge >> --abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated >> goal was providing consistency with other multi-command operations. >> >> I assume it would _just_ run a vanilla "git commit", and not try to do >> any trickery with updating the index (which could be disastrous). > > If we were to have "merge --continue", I agree that it would be the > logical implementation. > > There is nothing to "continue" in a stopped merge where Git asked > for help from the user, and because of that, I view the final "git > commit" as "concluding the merge", not "continuing". "continue" > makes quite a lot of sense with rebase and cherry-pick A..B that > stopped; it concludes the current step and let it continue to > process the remainder. So from that point of view, it somewhat > feels strange to call it "merge --continue", but it probably is just > me. > Yeah I did think that --continue wasn't quite the right word. git merge --conclude would probably be the most accurate.
Re: [BUG] Colon in remote urls
On Fri, Dec 09, 2016 at 10:32:48PM +0100, Johannes Sixt wrote: > > + if (*p == '\\') > > + literal = 1; > > There are too many systems out there that use a backslash in path names. I > don't think it is wise to use it also as the quoting character. Yeah, I picked it arbitrarily as the common quoting character, but I agree it probably makes backwards compatibility (and general usability when you have to double-backslash each instance) pretty gross on Windows. Most of the printable characters are going to suffer from backwards compatibility issues. Syntactically, we could use any ASCII control code below 0x20. Certainly our C code would be fine with that, but it seems pretty nasty. There's probably some crazy non-printing UTF-8 sequence we could use, too, but I'm not sure I want to go there. -Peff
Re: [PATCH 2/2] mergetools/tortoisemerge: simplify can_diff() by using "false"
Am 10.12.2016 um 04:21 schrieb David Aguilar:
Signed-off-by: David Aguilar
---
This patch builds upon da/mergetool-trust-exit-code
mergetools/tortoisemerge | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index d7ab666a59..9067d8a4e5 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -1,5 +1,5 @@
can_diff () {
- return 1
+ false
}
Why is this a simplification?
My concern is that 'false' is not necessarily a shell built-in. Then
this is actually a pessimization.
-- Hannes
