* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Junio C Hamano @ 2013-01-27 17:15 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Torsten Bögershausen, git, kraai
In-Reply-To: <20130127093121.GA4228@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> ...
> With the above definition of "which", the only sign of a mistake would
> be some extra output to stderr (which is quelled when running tests in
> the normal way). The "exit" is caught by the subshell and just makes
> the "if" condition false.
>
> That's not so terrible --- it could still dissuade new test authors
> from using "which". The downside I'd worry about is that it provides
> a false sense of security despite not catching problems ...
> ...
> In the end the analysis that works best would probably involve a
> full-fledged shell script parser. Something like "sparse", except for
> shell command language.
Exactly.
That is why I keep saying that whole test-lint-shell-syntax should
stay outside the default until it gets more robust by becoming a
reasonable shell parser; it may not necessarily have to become
"full" parser though.
As we discourage the use of tricky features of the language like
eval in individual test scripts to implement their own mini test
framework, the "something like sparse" parser may initialy start
small and still be useful; for example it can learn to exclude
anything inside <<HERE_DOCUMENT from getting inspected.
^ permalink raw reply
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Junio C Hamano @ 2013-01-27 17:34 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Jonathan Nieder, git, kraai
In-Reply-To: <5105280A.80002@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> Back to the which:
> ...
> and running "make test" gives the following, at least in my system:
> ...
I think everybody involved in this discussion already knows that;
the point is that it can easily give false negative, without the
scripts working very hard to do so.
If we did not care about incurring runtime performance cost, we
could arrange:
- the test framework to define a variable $TEST_ABORT that has a
full path to a file that is in somewhere test authors cannot
touch unless they really try hard to (i.e. preferrably outside
$TRASH_DIRECTORY, as it is not uncommon for to tests to do "rm *"
there). This location should be per $(basename "$0" .sh) to allow
running multiple tests in paralell;
- the test framework to "rm -f $TEST_ABORT" at the beginning of
test_expect_success/failure;
- test_expect_success/failure to check $TEST_ABORT and if it
exists, abort the execution, showing the contents of the file as
an error message.
Then you can wrap commands whose use we want to limit, perhaps like
this, in the test framework:
which () {
cat >"$TEST_ABORT" <<-\EOF
Do not use unportable 'which' in the test script.
"if type $cmd" is a good way to see if $cmd exists.
EOF
}
sed () {
saw_wantarg= must_abort=
for arg
do
if test -n "$saw_wantarg"
then
saw_wantarg=
continue
fi
case "$arg" in
--) break ;; # end of options
-i) echo >"$TEST_ABORT" "Do not use 'sed -i'"
must_abort=yes
break ;;
-e) saw_wantarg=yes ;; # skip next arg
-*) continue ;; # options without arg
*) break ;; # filename
esac
done
if test -z "$must_abort"
sed "$@"
fi
}
Then you can check that TEST_ABORT does not appear in test scripts
(ensuring that they do not attempt to circumvent the mechanis) and
catch use of unwanted commands or unwanted extended features of
commands at runtime.
But this will incur runtime performace hit, so I am not sure it
would be worth it.
^ permalink raw reply
* Re: [PATCH 2/2] mergetools: Make tortoisemerge work with
From: Junio C Hamano @ 2013-01-27 17:48 UTC (permalink / raw)
To: Sven Strickroth; +Cc: git, David Aguilar, Sebastian Schuberth, Jeff King
In-Reply-To: <5104F009.5020606@tu-clausthal.de>
Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:
> Am 26.01.2013 08:10 schrieb David Aguilar:
>> These patches look correct (I do not have the tool to test)
>> but I think we should fixup this commit message.
>>
>> How about something like...
>>
>> mergetools: Teach tortoisemerge about TortoiseGitMerge
>>
>> TortoiseGitMerge improved its syntax to allow for file paths
>> with spaces. Detect when it is installed and prefer it over
>> TortoiseMerge.
>
> This message implies, that I have to combine two patches again?!
We can see that [1/2] teaches mergetool to use tortoisegitmerge when
the user tells it to use tortoisemerge and the former is available.
The change in [2/2] is to use -base "$BASE" instead of -base:"$BASE"
when the real tool is tortoisegitmerge (when it is tortoisemerge,
nothing changes).
By reading these two patches, I would imagine that tortoisegitmerge
can accept both forms, i.e. -base:"$BASE" and -base "$BASE", but the
patch [2/2] considers that the latter form is preferrable in some
way. As you talked about "paths with SPs in them", I would imagine
that is the difference? That is -base:"$BASE" form will not work if
the varilable $BASE has a SP in it (even though it is encolosed in
dq, which does not make much sense from my POSIXy point of view, but
perhaps command line argument processing in the Windows land may
have different rules) but if you write -base "$BASE" then "$BASE"
will be taken as a single thing even it has a SP in it? Also I
would guess that the reason why patch [2/2] does this only for
tortoisegitmerge is either because tortoisemerge will break paths
with SPs even if it is given -base "$BASE" form, or because it only
accepts -base:"$BASE" form? I cannot read it from your description,
but let's assume that is the reason.
If that is the case, then the log message for the second patch would
be easier to understand if it says so in a more explicit way,
perhaps like this:
TortoiseGitMerge, unlike TortoiseMerge, can be told to
handle paths with SPs in them by using -option "$FILE" (not
-option:"$FILE", which does not work for such paths) syntax.
Use it to allow such paths to be handled correctly.
But I cannot read exactly why the patch [2/2] considers -base "$BASE"
is preferrable over -base:"$BASE" from your original description, so
this may well be way off the mark.
In short, I think proposed log message for [2/2] was not clear what
is being fixed and how.
^ permalink raw reply
* Re: Behavior of stash apply vs merge
From: Junio C Hamano @ 2013-01-27 17:57 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: Git List
In-Reply-To: <61173190.1700449.1359286556865.JavaMail.root@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> What good reason is it that 'git stash apply' gives hairy conflict
> markers, while 'git merge stash' does not. No renames involved.
"git merge stash" is nonsensical and would do a vastly different
thing compared to "git stash apply" depending on where you created
the stash and where you are attempting to run that operation.
Imagine you started fixing a bug while on the 'master' branch,
realized that the fix equally well applies to your 'maint' branch.
You would do "git stash" followed by "git checkout maint".
A sane person would do "git stash apply" at this point. It applies
the difference between the 'master' you were working on and your WIP
on top of your 'maint'.
"git merge stash" is entirely different. The history leading to a
stash looks like this:
I
/ \
---o---o---B---W
where
B is the commit you were working on (i.e. 'master');
I records the state of the index;
W records the state of the working tree.
and "stash" refers to W.
Think what commit B is in this example and the reason why you should
never ever do "git merge stash" will become apparent. By merging W
into 'maint', you would be pulling the entire history between
'maint' and 'master' to the result.
^ permalink raw reply
* Re: mergetool: include custom tools in '--tool-help'
From: Junio C Hamano @ 2013-01-27 18:03 UTC (permalink / raw)
To: John Keeping; +Cc: git, David Aguilar
In-Reply-To: <20130127163442.GQ7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> 'git mergetool --tool-help' only lists builtin tools, not those that the
> user has configured via a 'mergetool.<tool>.cmd' config value. Fix this
> by inspecting the tools configured in this way and adding them to the
> available and unavailable lists before displaying them.
Although I am not a mergetool user, I would imagine that it would
make sense to show it as available.
Just like "git help -a" lists subcommands in a way that can be easy
to tell which ones are the standard ones and which ones are user
customizations, this may want to give a similar distinction, though.
I dunno.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> After the recent changes to mergetool, do we want to do something like
> this as well, so that 'git mergetool --tool-help' will display any tools
> configured by the user/system administrator?
>
> This is on top of jk/mergetool.
>
> git-mergetool--lib.sh | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 1d0fb12..f9a617c 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -206,6 +206,29 @@ list_merge_tool_candidates () {
> esac
> }
>
> +# Adds tools from git-config to the available and unavailable lists.
> +# The tools are found in "$1.<tool>.cmd".
> +add_config_tools() {
> + section=$1
> +
> + eval $(git config --get-regexp $section'\..*\.cmd' |
> + while read -r key value
> + do
> + tool=${key#mergetool.}
> + tool=${tool%.cmd}
> +
> + tool=$(echo "$tool" |sed -e 's/'\''/'\''\\'\'\''/g')
> +
> + cmd=$(eval -- "set -- $value"; echo "$1")
> + if type "$cmd" >/dev/null 2>&1
> + then
> + echo "available=\"\${available}\"'$tool'\"\$LF\""
> + else
> + echo "unavailable=\"\${unavailable}\"'$tool'\"\$LF\""
> + fi
> + done)
> +}
> +
> show_tool_help () {
> unavailable= available= LF='
> '
> @@ -223,6 +246,12 @@ show_tool_help () {
> fi
> done
>
> + add_config_tools mergetool
> + if diff_mode
> + then
> + add_config_tools difftool
> + fi
> +
> cmd_name=${TOOL_MODE}tool
> if test -n "$available"
> then
^ permalink raw reply
* Re: [git-multimail] License unknown (#1)
From: Michael Haggerty @ 2013-01-27 18:52 UTC (permalink / raw)
To: git discussion list, Andy Parkins; +Cc: mhagger/git-multimail, Michiel Holtkamp
In-Reply-To: <mhagger/git-multimail/issues/1/12754195@github.com>
I have a question about the license of contrib/hooks/post-commit-email.
I had assumed that since it is in the git project, which is GPLv2, and
since it contains no contrary information, it would by implication also
fall under GPLv2. But the file itself contains no explicit license
information, and it is not clear to me that the "signed-off-by" line
implies a particular license, either. (The signed-off-by *does* seem to
imply that the source code is under some kind of open source license,
but not which one.)
If somebody can explain what license the code is under and how they come
to that conclusion, I would be very grateful.
And if Andy Parkins (the original author) is listening, please indicate
whether you had any intent *other* than GPLv2.
For anybody who is interested, the file was first committed in
4557e0de5b and has been modified by several authors since then.
Given the pretty clear open-sourciness of the script, I don't think this
has to be made into a big issue. But it would be nice to state the
license explicitly for future users' information.
Thanks,
Michael
On 01/27/2013 02:38 PM, Michiel Holtkamp wrote:
> Actually, I'm not sure that it is GPLv2 for the original script. The
> COPYING file in the main project declares the project as GPLv2, but it
> also says that people contributing should make their preferences (for
> licensing) known. Maybe we can assume it's GPLv2, (as the original
> writer might have assumed it was GPLv2), but it's not explicitly stated
> so I'm not sure (IANAL).
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH v4 1/2] for-each-repo: new command used for multi-repo operations
From: Junio C Hamano @ 2013-01-27 19:04 UTC (permalink / raw)
To: Lars Hjemli; +Cc: git
In-Reply-To: <1359290777-5483-2-git-send-email-hjemli@gmail.com>
Lars Hjemli <hjemli@gmail.com> writes:
> When working with multiple, unrelated (or loosly related) git repos,
> there is often a need to locate all repos with uncommitted work and
> perform some action on them (say, commit and push). Before this patch,
> such tasks would require manually visiting all repositories, running
> `git status` within each one and then decide what to do next.
>
> This mundane task can now be automated by e.g. `git for-each-repo --dirty
> status`, which will find all non-bare git repositories below the current
> directory (even nested ones), check if they are dirty (as defined by
> `git diff --quiet && git diff --cached --quiet`), and for each dirty repo
> print the path to the repo and then execute `git status` within the repo.
>
> The command also honours the option '--clean' which restricts the set of
> repos to those which '--dirty' would skip, and '-x' which is used to
> execute non-git commands.
It might make sense to internally use RUN_GIT_CMD flag when the
first word of the command line is 'git' as an optimization, but
I am not sure it is a good idea to force the end users to think
when to use -x and when not to is a good idea.
In other words, I think
git for-each-repo -d diff --name-only
git for-each-repo -d -x ls '*.c'
is less nice than letting the user say
git for-each-repo -d git diff --name-only
git for-each-repo -d ls '*.c'
> Finally, the command to execute within each repo is optional. If none is
> given, git-for-each-repo will just print the path to each repo found. And
> since the command supports -z, this can be used for more advanced scripting
> needs.
It amounts to the same thing, but I would rather describe it as:
To allow scripts to handle paths with shell-unsafe characters,
support "-z" to show paths with NUL termination. Otherwise,
such paths are shown with the usual c-quoting.
One more thing that nobody brought up during the previous reviews is
if we want to support subset of repositories by allowing the
standard pathspec match mechanism. For example,
git for-each-repo -d git diff --name-only -- foo/ bar/b\*z
might be a way to ask "please find repositories match the given
pathspecs (i.e. foo/ bar/b\*z) and run the command in the ones that
are dirty". We would need to think about how to mark the end of the
command though---we could borrow \; from find(1), even though find
is not the best example of the UI design. I.e.
git for-each-repo -d git diff --name-only \; [--] foo/ bar/b\*z
with or without "--".
> diff --git a/Documentation/git-for-each-repo.txt b/Documentation/git-for-each-repo.txt
> new file mode 100644
> index 0000000..fb12b3f
> --- /dev/null
> +++ b/Documentation/git-for-each-repo.txt
> @@ -0,0 +1,71 @@
> +git-for-each-repo(1)
> +====================
> +
> +NAME
> +----
> +git-for-each-repo - Execute a git command in multiple non-bare repositories
There is a separate topic in flight that turns s/git/Git/ when we
refer to the system as a whole. In any case, this is no longer
limited to "execute a Git command".
Find non-bare Git repositories in subdirectories
or
Find or execute a command in non-bare Git repositories in subdirectories
perhaps?
> +SYNOPSIS
> +--------
> +[verse]
> +'git for-each-repo' [-acdxz] [command]
> +
> +DESCRIPTION
> +-----------
> +The git-for-each-repo command is used to locate all non-bare git
Should be sufficient to say s/is used to locate/locates/.
> +repositories within the current directory tree, and optionally
> +execute a git command in each of the found repos.
s/a git command/a command/;
> +OPTIONS
> +-------
> ...
> +-x::
> + Execute a genric (non-git) command in each repo.
Drop this option.
> +NOTES
> +-----
> +
> +For the purpose of `git-for-each-repo`, a dirty worktree is defined as a
> +worktree with uncommitted changes.
Is it a definition that is different from usual? If so why does it
need to be inconsistent with the rest of the system?
> diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
> new file mode 100644
> index 0000000..9333ae0
> --- /dev/null
> +++ b/builtin/for-each-repo.c
> @@ -0,0 +1,145 @@
> +/*
> + * "git for-each-repo" builtin command.
> + *
> + * Copyright (c) 2013 Lars Hjemli <hjemli@gmail.com>
> + */
> +#include "cache.h"
> +#include "color.h"
> +#include "quote.h"
> +#include "builtin.h"
> +#include "run-command.h"
> +#include "parse-options.h"
> +
> +#define ALL 0
> +#define DIRTY 1
> +#define CLEAN 2
> +
> +static char *color = GIT_COLOR_NORMAL;
> +static int eol = '\n';
> +static int match;
> +static int runopt = RUN_GIT_CMD;
> +
> +static const char * const builtin_foreachrepo_usage[] = {
> + N_("git for-each-repo [-acdxz] [cmd]"),
> + NULL
> +};
> +
> +static struct option builtin_foreachrepo_options[] = {
> + OPT_SET_INT('a', "all", &match, N_("match both clean and dirty repositories"), ALL),
> + OPT_SET_INT('c', "clean", &match, N_("only show clean repositories"), CLEAN),
> + OPT_SET_INT('d', "dirty", &match, N_("only show dirty repositories"), DIRTY),
> + OPT_SET_INT('x', NULL, &runopt, N_("execute generic (non-git) command"), 0),
> + OPT_SET_INT('z', NULL, &eol, N_("terminate each repo path with NUL character"), 0),
> + OPT_END(),
> +};
> +
> +static int get_repo_state(const char *dir)
> +{
> + const char *diffidx[] = {"diff", "--quiet", "--cached", NULL};
> + const char *diffwd[] = {"diff", "--quiet", NULL};
> +
> + if (run_command_v_opt_cd_env(diffidx, RUN_GIT_CMD, dir, NULL) != 0)
> + return DIRTY;
> + if (run_command_v_opt_cd_env(diffwd, RUN_GIT_CMD, dir, NULL) != 0)
> + return DIRTY;
> + return CLEAN;
> +}
> +
> +static void print_repo_path(const char *path, unsigned pretty)
> +{
> + if (path[0] == '.' && path[1] == '/')
> + path += 2;
> + if (pretty)
> + color_fprintf_ln(stdout, color, "[%s]", path);
This is shown before running a command in that repository. I am of
two minds. It certainly is nice to be able to tell which repository
each block of output lines comes from, and not requiring the command
to do this themselves is a good default. However, I wonder if people
would want to do something like this:
git for-each-repo sh -c '
git diff --name-only |
sed -e "s|^|$path/|"
'
to get a consolidated view, in a way similar to how "submodule
foreach" can be used. This unconditional output will get in the way
for such a use case.
Oh, that reminds me of another thing. Perhaps we would want to
export the (relative) path to the found repository in some way to
allow the commands to do this kind of thing in the first place?
"submodule foreach" does this with $path, I think.
> + else
> + write_name_quoted(path, stdout, eol);
> +}
Nice. Doubly nice that you do not hardcode "color" at this point
but made it into a separate variable.
> +static void handle_repo(struct strbuf *path, const char **argv)
> +{
> + const char *gitdir;
> + int len;
> +
> + len = path->len;
> + strbuf_addstr(path, ".git");
> + gitdir = resolve_gitdir(path->buf);
> + strbuf_setlen(path, len - 1);
> + if (!gitdir)
> + goto done;
> + if (match != ALL && match != get_repo_state(path->buf))
> + goto done;
> + print_repo_path(path->buf, *argv != NULL);
> + if (*argv)
> + run_command_v_opt_cd_env(argv, runopt, path->buf, NULL);
> +done:
> + strbuf_addstr(path, "/");
OK, you get "$D/" from the caller, make it "$D/.git" to call
resolve_gitdir() with, turn it to "$D" before printing and runnning,
and then add "/" back. Slightly tricky but correct.
> +static int walk(struct strbuf *path, int argc, const char **argv)
> +{
> + DIR *dir;
> + struct dirent *ent;
> + struct stat st;
> + size_t len;
> + int has_dotgit = 0;
> + struct string_list list = STRING_LIST_INIT_DUP;
> + struct string_list_item *item;
> +
> + dir = opendir(path->buf);
> + if (!dir)
> + return errno;
> + strbuf_addstr(path, "/");
> + len = path->len;
> + while ((ent = readdir(dir))) {
> + if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
> + continue;
> + if (!strcmp(ent->d_name, ".git")) {
> + has_dotgit = 1;
> + continue;
> + }
> + switch (DTYPE(ent)) {
> + case DT_UNKNOWN:
> + case DT_LNK:
> + /* Use stat() to figure out if this path leads
> + * to a directory - it's not important if it's
> + * a symlink which gets us there.
> + */
> + strbuf_setlen(path, len);
> + strbuf_addstr(path, ent->d_name);
> + if (stat(path->buf, &st) || !S_ISDIR(st.st_mode))
> + break;
> + /* fallthrough */
> + case DT_DIR:
> + string_list_append(&list, ent->d_name);
> + break;
> + }
> + }
> + closedir(dir);
> + strbuf_setlen(path, len);
> + if (has_dotgit)
> + handle_repo(path, argv);
> + sort_string_list(&list);
> + for_each_string_list_item(item, &list) {
> + strbuf_setlen(path, len);
> + strbuf_addstr(path, item->string);
> + walk(path, argc, argv);
> + }
> + string_list_clear(&list, 0);
> + return 0;
> +}
Is the "collect-first-and-then-sort" done so that the repositories
are shown in a stable order regardless of the order in which
readdir() returns he entries? I am not complaining, but being
curious.
> diff --git a/t/t6400-for-each-repo.sh b/t/t6400-for-each-repo.sh
This command does not look like "6 - the revision tree commands" to
me. "7 - the porcelainish commands concerning the working tree" or
"9 - the git tools" may be a better match?
> new file mode 100755
> index 0000000..af02c0c
> --- /dev/null
> +++ b/t/t6400-for-each-repo.sh
> @@ -0,0 +1,150 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2013 Lars Hjemli
> +#
> +
> +test_description='Test the git-for-each-repo command'
> +
> +. ./test-lib.sh
> +
> +qname="with\"quote"
> +qqname="\"with\\\"quote\""
If Windows does not have problems with paths with dq in it, then
this is fine, but I dunno. Otherwise, you may want to exclude the
c-quote testing from the main part of the test, and have a single
test that has prerequisite for filesystems that can do this at the
end of the script.
> +test_expect_success "setup" '
> + test_create_repo clean &&
> + (cd clean && test_commit foo1) &&
> + git init --separate-git-dir=.cleansub clean/gitfile &&
> + (cd clean/gitfile && test_commit foo2 && echo bar >>foo2.t) &&
> + test_create_repo dirty-idx &&
> + (cd dirty-idx && test_commit foo3 && git rm foo3.t) &&
> + test_create_repo dirty-wt &&
> + (cd dirty-wt && mv .git .linkedgit && ln -s .linkedgit .git &&
Some platforms are symlink-challenged. Can we do this test without
"ln -s"? SYMLINKS prereq wouldn't be very useful for the setup
step, as all the remaining tests won't work without setting up the
test scenario.
> + test_commit foo4 && rm foo4.t) &&
> + test_create_repo "$qname" &&
> + (cd "$qname" && test_commit foo5) &&
> + mkdir fakedir && mkdir fakedir/.git
> +'
> +
> +test_expect_success "without filtering, all repos are included" '
> + echo "." >expect &&
> + echo "clean" >>expect &&
> + echo "clean/gitfile" >>expect &&
> + echo "dirty-idx" >>expect &&
> + echo "dirty-wt" >>expect &&
> + echo "$qqname" >>expect &&
A single
cat >expect <<-EOF
.
clean
clean/gitfile
...
$qqname
EOF
may be a lot easier to read (likewise for all the "expect"
preparation in the rest of the script).
> +test_expect_success "-z NUL-terminates each path" '
> + echo "(.)" >expect &&
> + echo "(clean)" >>expect &&
> + echo "(clean/gitfile)" >>expect &&
> + echo "(dirty-idx)" >>expect &&
> + echo "(dirty-wt)" >>expect &&
> + echo "($qname)" >>expect &&
> + git for-each-repo -z | xargs -0 printf "(%s)\n" >actual &&
This needs prereq on "xargs -0", but because we know we do not have
any string with Q in it in the expected list of repositories, it may
be simpler to do something like this:
echo ".QcleanQclean/gitfileQ...$qname" >expect &&
git for-each-repo -z | tr "\0" Q >actual &&
test_cmp expect actual
Thanks.
^ permalink raw reply
* Re: Behavior of stash apply vs merge
From: Robin Rosenberg @ 2013-01-27 19:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <7vvcaiwltj.fsf@alter.siamese.dyndns.org>
Thanks. Feeling a bit studid now.
I was actually thinking about using merge to implement stash apply
in JGit. What we have is broken so I tried using merge to implement
it and them compared to git merge --no-commit.. FAIL.
The main difference is of course that I set the merge base to
stash^1, which is obviously not what my question was about.
-- robin
^ permalink raw reply
* Re: Behavior of stash apply vs merge
From: Junio C Hamano @ 2013-01-27 19:12 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: Git List
In-Reply-To: <2043716001.1806075.1359313796808.JavaMail.root@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> Thanks. Feeling a bit studid now.
>
> I was actually thinking about using merge to implement stash apply
> in JGit. What we have is broken so I tried using merge to implement
> it and them compared to git merge --no-commit.. FAIL.
Do you have "cherry-pick"?
In short, "stash apply" is a "cherry-pick" in disguise.
^ permalink raw reply
* Re: [PATCH v4 1/2] for-each-repo: new command used for multi-repo operations
From: John Keeping @ 2013-01-27 19:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Hjemli, git
In-Reply-To: <7vk3qywiqf.fsf@alter.siamese.dyndns.org>
On Sun, Jan 27, 2013 at 11:04:08AM -0800, Junio C Hamano wrote:
> One more thing that nobody brought up during the previous reviews is
> if we want to support subset of repositories by allowing the
> standard pathspec match mechanism. For example,
>
> git for-each-repo -d git diff --name-only -- foo/ bar/b\*z
>
> might be a way to ask "please find repositories match the given
> pathspecs (i.e. foo/ bar/b\*z) and run the command in the ones that
> are dirty". We would need to think about how to mark the end of the
> command though---we could borrow \; from find(1), even though find
> is not the best example of the UI design. I.e.
>
> git for-each-repo -d git diff --name-only \; [--] foo/ bar/b\*z
>
> with or without "--".
Would it be better to make this a (multi-valued) option?
git for-each-repo -d --filter=foo/ --filter=bar/b\*z git diff --name-only
It seems a lot simpler than trying to figure out how the command is
going to handle '--' arguments.
> Oh, that reminds me of another thing. Perhaps we would want to
> export the (relative) path to the found repository in some way to
> allow the commands to do this kind of thing in the first place?
> "submodule foreach" does this with $path, I think.
I think $path is the only variable exported by "submodule foreach" which
is applicable here, but it doesn't work on Windows, where environment
variables are case-insensitive.
Commit 64394e3 (git-submodule.sh: Don't use $path variable in
eval_gettext string) changed "submodule foreach" to use $sm_path
internally although I notice that the documentation still uses $path.
Perhaps $repo_path in this case?
John
^ permalink raw reply
* Re: [PATCH v4 1/2] for-each-repo: new command used for multi-repo operations
From: Junio C Hamano @ 2013-01-27 19:45 UTC (permalink / raw)
To: John Keeping; +Cc: Lars Hjemli, git
In-Reply-To: <20130127194223.GR7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> On Sun, Jan 27, 2013 at 11:04:08AM -0800, Junio C Hamano wrote:
>> One more thing that nobody brought up during the previous reviews is
>> if we want to support subset of repositories by allowing the
>> standard pathspec match mechanism. For example,
>>
>> git for-each-repo -d git diff --name-only -- foo/ bar/b\*z
>>
>> might be a way to ask "please find repositories match the given
>> pathspecs (i.e. foo/ bar/b\*z) and run the command in the ones that
>> are dirty". We would need to think about how to mark the end of the
>> command though---we could borrow \; from find(1), even though find
>> is not the best example of the UI design. I.e.
>>
>> git for-each-repo -d git diff --name-only \; [--] foo/ bar/b\*z
>>
>> with or without "--".
>
> Would it be better to make this a (multi-valued) option?
>
> git for-each-repo -d --filter=foo/ --filter=bar/b\*z git diff --name-only
The standard way to use filtering based on paths we have is to use
the pathspec parameters at the end of the commmand line.
I see no reason for such an inconsistency with an option like --filter.
>> Oh, that reminds me of another thing. Perhaps we would want to
>> export the (relative) path to the found repository in some way to
>> allow the commands to do this kind of thing in the first place?
>> "submodule foreach" does this with $path, I think.
>
> I think $path is the only variable exported by "submodule foreach" which
> is applicable here, but it doesn't work on Windows, where environment
> variables are case-insensitive.
>
> Commit 64394e3 (git-submodule.sh: Don't use $path variable in
> eval_gettext string) changed "submodule foreach" to use $sm_path
> internally although I notice that the documentation still uses $path.
>
> Perhaps $repo_path in this case?
I do not care too deeply about the name, as long as the names used
by both mechanisms are the same.
^ permalink raw reply
* Re: [PATCH] git-remote-testpy: fix patch hashing on Python 3
From: Junio C Hamano @ 2013-01-27 19:49 UTC (permalink / raw)
To: John Keeping; +Cc: Michael Haggerty, git, Sverre Rabbelier
In-Reply-To: <20130127145056.GP7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> When this change was originally made (0846b0c - git-remote-testpy: hash
> bytes explicitly , I didn't realised that the "hex" encoding we chose is
> a "bytes to bytes" encoding so it just fails with an error on Python 3
> in the same way as the original code.
>
> It is not possible to provide a single code path that works on Python 2
> and Python 3 since Python 2.x will attempt to decode the string before
> encoding it, which fails for strings that are not valid in the default
> encoding. Python 3.1 introduced the "surrogateescape" error handler
> which handles this correctly and permits a bytes -> unicode -> bytes
> round-trip to be lossless.
>
> At this point Python 3.0 is unsupported so we don't go out of our way to
> try to support it.
>
> Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
Thanks; will queue and wait for an Ack from Michael.
Does the helper function need to be named with leading underscore,
though?
> On Sun, Jan 27, 2013 at 02:13:29PM +0000, John Keeping wrote:
>> On Sun, Jan 27, 2013 at 05:44:37AM +0100, Michael Haggerty wrote:
>> > So to handle all of the cases across Python versions as closely as
>> > possible to the old 2.x code, it might be necessary to make the code
>> > explicitly depend on the Python version number, like:
>> >
>> > hasher = _digest()
>> > if sys.hexversion < 0x03000000:
>> > pathbytes = repo.path
>> > elif sys.hexversion < 0x03010000:
>> > # If support for Python 3.0.x is desired (note: result can
>> > # be different in this case than under 2.x or 3.1+):
>> > pathbytes = repo.path.encode(sys.getfilesystemencoding(),
>> > 'backslashreplace')
>> > else
>> > pathbytes = repo.path.encode(sys.getfilesystemencoding(),
>> > 'surrogateescape')
>> > hasher.update(pathbytes)
>> > repo.hash = hasher.hexdigest()
>
> How about this?
>
> git-remote-testpy.py | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/git-remote-testpy.py b/git-remote-testpy.py
> index c7a04ec..16b0c52 100644
> --- a/git-remote-testpy.py
> +++ b/git-remote-testpy.py
> @@ -36,6 +36,22 @@ if sys.hexversion < 0x02000000:
> sys.stderr.write("git-remote-testgit: requires Python 2.0 or later.\n")
> sys.exit(1)
>
> +
> +def _encode_filepath(path):
> + """Encodes a Unicode file path to a byte string.
> +
> + On Python 2 this is a no-op; on Python 3 we encode the string as
> + suggested by [1] which allows an exact round-trip from the command line
> + to the filesystem.
> +
> + [1] http://docs.python.org/3/c-api/unicode.html#file-system-encoding
> +
> + """
> + if sys.hexversion < 0x03000000:
> + return path
> + return path.encode('utf-8', 'surrogateescape')
> +
> +
> def get_repo(alias, url):
> """Returns a git repository object initialized for usage.
> """
> @@ -45,7 +61,7 @@ def get_repo(alias, url):
> repo.get_head()
>
> hasher = _digest()
> - hasher.update(repo.path.encode('hex'))
> + hasher.update(_encode_filepath(repo.path))
> repo.hash = hasher.hexdigest()
>
> repo.get_base_path = lambda base: os.path.join(
^ permalink raw reply
* Re: Behavior of stash apply vs merge
From: Robin Rosenberg @ 2013-01-27 19:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <7vfw1mwibu.fsf@alter.siamese.dyndns.org>
----- Ursprungligt meddelande -----
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
>
> > Thanks. Feeling a bit studid now.
> >
> > I was actually thinking about using merge to implement stash apply
> > in JGit. What we have is broken so I tried using merge to implement
> > it and them compared to git merge --no-commit.. FAIL.
>
> Do you have "cherry-pick"?
>
> In short, "stash apply" is a "cherry-pick" in disguise.
Yes, that's what I did. Thanks for confirming this. One for the working
tree and if that succeeds I do another one to restore the index if requested.
-- robin
^ permalink raw reply
* Re: mergetool: include custom tools in '--tool-help'
From: John Keeping @ 2013-01-27 19:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, David Aguilar
In-Reply-To: <7vobgawljs.fsf@alter.siamese.dyndns.org>
On Sun, Jan 27, 2013 at 10:03:19AM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > 'git mergetool --tool-help' only lists builtin tools, not those that the
> > user has configured via a 'mergetool.<tool>.cmd' config value. Fix this
> > by inspecting the tools configured in this way and adding them to the
> > available and unavailable lists before displaying them.
>
> Although I am not a mergetool user, I would imagine that it would
> make sense to show it as available.
>
> Just like "git help -a" lists subcommands in a way that can be easy
> to tell which ones are the standard ones and which ones are user
> customizations, this may want to give a similar distinction, though.
> I dunno.
I think I'd want to do this with a suffix if at all, so the output would
be like this:
'git mergetool --tool=<tool>' may be set to one of the following:
araxis
gvimdiff
gvimdiff2
mytool (user-defined)
vimdiff
vimdiff2
The following tools are valid, but not currently available:
bc3
codecompare
deltawalker
diffuse
ecmerge
emerge
kdiff3
meld
opendiff
p4merge
tkdiff
tortoisemerge
xxdiff
Some of the tools listed above only work in a windowed
environment. If run in a terminal-only session, they will fail.
Adding more sections for the user-defined tools feels like the output
would be too imposing and would make it hard to immediately identify the
valid option.
John
^ permalink raw reply
* Re: [PATCH] git-remote-testpy: fix patch hashing on Python 3
From: John Keeping @ 2013-01-27 20:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git, Sverre Rabbelier
In-Reply-To: <7vzjzuv224.fsf@alter.siamese.dyndns.org>
On Sun, Jan 27, 2013 at 11:49:39AM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > When this change was originally made (0846b0c - git-remote-testpy: hash
> > bytes explicitly , I didn't realised that the "hex" encoding we chose is
> > a "bytes to bytes" encoding so it just fails with an error on Python 3
> > in the same way as the original code.
> >
> > It is not possible to provide a single code path that works on Python 2
> > and Python 3 since Python 2.x will attempt to decode the string before
> > encoding it, which fails for strings that are not valid in the default
> > encoding. Python 3.1 introduced the "surrogateescape" error handler
> > which handles this correctly and permits a bytes -> unicode -> bytes
> > round-trip to be lossless.
> >
> > At this point Python 3.0 is unsupported so we don't go out of our way to
> > try to support it.
> >
> > Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
>
> Thanks; will queue and wait for an Ack from Michael.
>
> Does the helper function need to be named with leading underscore,
> though?
It's a Python convention for internal functions. Since this is a script
not a library module I don't feel strongly about it in this case.
John
^ permalink raw reply
* Re: [PATCH 2/2] fetch-pack: avoid repeatedly re-scanning pack directory
From: Junio C Hamano @ 2013-01-27 20:09 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jeff King, git
In-Reply-To: <20130127102753.GB4228@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Jeff King wrote:
>
>> When we look up a sha1 object for reading, we first check
>> packfiles, and then loose objects. If we still haven't found
>> it, we re-scan the list of packfiles in `objects/pack`. This
>> final step ensures that we can co-exist with a simultaneous
>> repack process which creates a new pack and then prunes the
>> old object.
>
> I like the context above and what follows it, but I think you forgot
> to mention what the patch actually does. :)
>
> I guess it is:
>
> However, in the first scan over refs in fetch-pack.c::everything_local,
> this double-check of packfiles is not necessary since we are only
> trying to get a rough estimate of the last time we fetched from this
> remote repository in order to find good candidate common commits ---
> a missed object would only result in a slightly slower fetch.
It is not about a rough estimate nor common commits, though. The
"everything local" check in question is interested in only one
thing: are we _clearly_ up to date without fetching anything from
them?
Loosening the check may miss the rare case where we race against a
simultaneous repack and will cause us to go to the network when we
do not have to, and it becomes a trade off between the common unracy
case going faster by allowing the "Are we clearly up to date" check
to cheat, at the expense of rare racy cases suffering unnecessary
object transfer overhead.
> Avoid that slow second scan in the common case by guarding the object
> lookup with has_sha1_file().
This conclusion is correct.
> I had not read this codepath before. I'm left with a few questions:
>
> * Why is 49bb805e ("Do not ask for objects known to be complete",
> 2005-10-19) trying to do? Are we hurting that in any way?
An earlier fetch may have acquired all the necessary objects but may
not have updated our refs for some reason (e.g. fast-forward check
may have fired). In such a case, we may already have a history that
is good (i.e. not missing paths down to the common history) in our
repository that is not connected to any of our refs, and we can
update our refs (or write to FETCH_HEAD) without asking the remote
end to do any common ancestor computation or object transfer.
That was the primary thing the patch wanted to do.
As a side-effect, we know more objects than just the objects at the
tips of our refs are complete and that may help the later common
history discovery step, but obviously we do not want to dig the
history down to root. The cutoff value is merely a heuristics
chosen without any deep thought.
> * Is has_sha1_file() generally succeptible to the race against repack
> you mentioned? How is that normally dealt with?
By failing to find, so that the user will restart. When the caller
really wants to use the object, parse_objects() => read_sha1_file()
=> read_object() is used and we will see the retry.
> * Can a slow operation get confused if an object is incorporated into
> a pack and then expelled again by two repacks in sequence?
If it checks "the object should be there" first, wait for a long
time, and then tries to find that object's data, the later access
will go to the parse_objects() callpath and I think it should do the
right thing. If that slow opearation stops inside read_object(), it
could find it unable to map the loose object file and then unable to
find it in the pack, either. Is that what you are worried about?
^ permalink raw reply
* Re: [PATCH] git-remote-testpy: fix patch hashing on Python 3
From: Junio C Hamano @ 2013-01-27 20:11 UTC (permalink / raw)
To: John Keeping; +Cc: Michael Haggerty, git, Sverre Rabbelier
In-Reply-To: <20130127200401.GT7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
>> Thanks; will queue and wait for an Ack from Michael.
>>
>> Does the helper function need to be named with leading underscore,
>> though?
>
> ... Since this is a script
> not a library module I don't feel strongly about it in this case.
That is exactly why I asked.
^ permalink raw reply
* Re: mergetool: include custom tools in '--tool-help'
From: Junio C Hamano @ 2013-01-27 20:13 UTC (permalink / raw)
To: John Keeping; +Cc: git, David Aguilar
In-Reply-To: <20130127195618.GS7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> I think I'd want to do this with a suffix if at all, so the output would
> be like this:
>
> 'git mergetool --tool=<tool>' may be set to one of the following:
>
> araxis
> gvimdiff
> gvimdiff2
> mytool (user-defined)
> vimdiff
> vimdiff2
That is fine by me, but the real users of mergetool please feel free
to raise objections.
^ permalink raw reply
* Re: [PATCH] git-remote-testpy: fix patch hashing on Python 3
From: John Keeping @ 2013-01-27 20:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git, Sverre Rabbelier
In-Reply-To: <7vr4l6v11z.fsf@alter.siamese.dyndns.org>
On Sun, Jan 27, 2013 at 12:11:20PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> >> Thanks; will queue and wait for an Ack from Michael.
> >>
> >> Does the helper function need to be named with leading underscore,
> >> though?
> >
> > ... Since this is a script
> > not a library module I don't feel strongly about it in this case.
>
> That is exactly why I asked.
So I think the answer is "habit, but I probably shouldn't have put it
in in this case".
John
^ permalink raw reply
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Junio C Hamano @ 2013-01-27 20:25 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Jonathan Nieder, git, kraai
In-Reply-To: <7v4ni2y1fm.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> If we did not care about incurring runtime performance cost, we
> could arrange:
> ...
> Then you can wrap commands whose use we want to limit, perhaps like
> this, in the test framework:
> ...
> sed () {
> ...
> done
> if test -z "$must_abort"
> sed "$@"
> fi
> }
Of course, aside from missing "then", this needs to use the
real "sed", so this has to be
if test -z "$must_abort"
then
command sed "$@"
fi
or something like that.
An approach along this line may reduce both the false negatives and
false positives down to an acceptable level, but I doubt the result
would be efficient enough for us to tolerate the runtime penalty.
^ permalink raw reply
* Re: [PATCH v2] add: warn when -u or -A is used without filepattern
From: Junio C Hamano @ 2013-01-27 20:33 UTC (permalink / raw)
To: Matthieu Moy
Cc: git, Jonathan Nieder, Robin Rosenberg, Piotr Krukowiecki,
Eric James Michael Ritz, Tomas Carnecky
In-Reply-To: <vpqtxq28v3s.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Plus, option_with_implicit_dot is used in cut-and-paste ready commands
> below.
I do not think we should aim for easy cut-and-paste, especially when
the real purpose of the change is to train people's fingers; the
message should discouraging cut-and-paste in a case like this, if
anything.
But we could obviously do this, if you really want to cut-and-paste.
builtin/add.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 7552f7f..ba72a57 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -363,7 +363,7 @@ static int add_files(struct dir_struct *dir, int flags)
return exit_status;
}
-static void warn_pathless_add(const char *option_name) {
+static void warn_pathless_add(const char *option_name, const char *short_name) {
/*
* To be consistent with "git add -p" and most Git
* commands, we should default to being tree-wide, but
@@ -374,20 +374,21 @@ static void warn_pathless_add(const char *option_name) {
* turned into a die(...), and eventually we may
* reallow the command with a new behavior.
*/
- warning(_("The behavior of 'git add %s' with no path argument from a subdirectory of the\n"
- "tree will change in Git 2.0 and shouldn't be used anymore.\n"
+ warning(_("The behavior of 'git add %s (or %s)' with no path argument from a\n"
+ "subdirectory of the tree will change in Git 2.0 and should not be\n"
+ "used anymore.\n"
"To add content for the whole tree, run:\n"
"\n"
- " git add %s :/\n"
+ " git add %s :/ ;# or git add %s :/\n"
"\n"
"To restrict the command to the current directory, run:\n"
"\n"
- " git add %s .\n"
+ " git add %s . ;# or git add %s .\n"
"\n"
"With the current Git version, the command is restricted to the current directory."),
- option_name,
- option_name,
- option_name);
+ option_name, short_name,
+ option_name, short_name,
+ option_name, short_name);
}
int cmd_add(int argc, const char **argv, const char *prefix)
@@ -401,6 +402,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int require_pathspec;
char *seen = NULL;
const char *option_with_implicit_dot = NULL;
+ const char *short_option_with_implicit_dot = NULL;
git_config(add_config, NULL);
@@ -420,14 +422,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
die(_("-A and -u are mutually incompatible"));
if (!show_only && ignore_missing)
die(_("Option --ignore-missing can only be used together with --dry-run"));
- if (addremove)
+ if (addremove) {
option_with_implicit_dot = "--all";
- if (take_worktree_changes)
+ short_option_with_implicit_dot = "-A";
+ }
+ if (take_worktree_changes) {
option_with_implicit_dot = "--update";
+ short_option_with_implicit_dot = "-u";
+ }
if (option_with_implicit_dot && !argc) {
static const char *here[2] = { ".", NULL };
if (prefix)
- warn_pathless_add(option_with_implicit_dot);
+ warn_pathless_add(option_with_implicit_dot,
+ short_option_with_implicit_dot);
argc = 1;
argv = here;
}
^ permalink raw reply related
* Re: [PATCH] git-remote-testpy: fix patch hashing on Python 3
From: Junio C Hamano @ 2013-01-27 20:38 UTC (permalink / raw)
To: John Keeping; +Cc: Michael Haggerty, git, Sverre Rabbelier
In-Reply-To: <20130127202106.GU7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> On Sun, Jan 27, 2013 at 12:11:20PM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>> >> Thanks; will queue and wait for an Ack from Michael.
>> >>
>> >> Does the helper function need to be named with leading underscore,
>> >> though?
>> >
>> > ... Since this is a script
>> > not a library module I don't feel strongly about it in this case.
>>
>> That is exactly why I asked.
>
> So I think the answer is "habit, but I probably shouldn't have put it
> in in this case".
OK, then I'll queue with a local amend to drop the leading
underscore.
Thanks.
^ permalink raw reply
* Re: [PATCH] git-remote-testpy: fix patch hashing on Python 3
From: Junio C Hamano @ 2013-01-27 20:47 UTC (permalink / raw)
To: John Keeping; +Cc: Michael Haggerty, git, Sverre Rabbelier
In-Reply-To: <7va9ruuzsf.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> John Keeping <john@keeping.me.uk> writes:
>
>> So I think the answer is "habit, but I probably shouldn't have put it
>> in in this case".
>
> OK, then I'll queue with a local amend to drop the leading
> underscore.
So this is what I will be queuing (I'd appreciate the second set of
eyes, though), with the leading-underscore removal and log message
typofixes.
I remember that I earlier asked somewhere if we want to say "Python
3.x that is older than 3.y is unsupported"
http://thread.gmane.org/gmane.comp.version-control.git/213920/focus=213926
but I was told that we will support all versions in 3.x series, IIRC.
Does this patch contradict with that? If so I think we would need
to revisit the update to CodingGuidelines in that thread.
I am perfectly fine with discarding early 3.x as "0.x releases of
Python3", but I would want to see our document say so if that is
what we do.
-- >8 --
From: John Keeping <john@keeping.me.uk>
Date: Sun, 27 Jan 2013 14:50:56 +0000
Subject: [PATCH] git-remote-testpy: fix path hashing on Python 3
When this change was originally made (0846b0c - git-remote-testpy:
hash bytes explicitly , I didn't realise that the "hex" encoding we
chose is a "bytes to bytes" encoding so it just fails with an error
on Python 3 in the same way as the original code.
It is not possible to provide a single code path that works on
Python 2 and Python 3 since Python 2.x will attempt to decode the
string before encoding it, which fails for strings that are not
valid in the default encoding. Python 3.1 introduced the
"surrogateescape" error handler which handles this correctly and
permits a bytes -> unicode -> bytes round-trip to be lossless.
At this point Python 3.0 is unsupported so we don't go out of our
way to try to support it.
Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-remote-testpy.py | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/git-remote-testpy.py b/git-remote-testpy.py
index c7a04ec..6098bdd 100644
--- a/git-remote-testpy.py
+++ b/git-remote-testpy.py
@@ -36,6 +36,22 @@ if sys.hexversion < 0x02000000:
sys.stderr.write("git-remote-testgit: requires Python 2.0 or later.\n")
sys.exit(1)
+
+def encode_filepath(path):
+ """Encodes a Unicode file path to a byte string.
+
+ On Python 2 this is a no-op; on Python 3 we encode the string as
+ suggested by [1] which allows an exact round-trip from the command line
+ to the filesystem.
+
+ [1] http://docs.python.org/3/c-api/unicode.html#file-system-encoding
+
+ """
+ if sys.hexversion < 0x03000000:
+ return path
+ return path.encode('utf-8', 'surrogateescape')
+
+
def get_repo(alias, url):
"""Returns a git repository object initialized for usage.
"""
@@ -45,7 +61,7 @@ def get_repo(alias, url):
repo.get_head()
hasher = _digest()
- hasher.update(repo.path.encode('hex'))
+ hasher.update(encode_filepath(repo.path))
repo.hash = hasher.hexdigest()
repo.get_base_path = lambda base: os.path.join(
--
1.8.1.1.550.g40037fd
^ permalink raw reply related
* Re: mergetool: include custom tools in '--tool-help'
From: David Aguilar @ 2013-01-27 21:10 UTC (permalink / raw)
To: John Keeping; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <7vmwvuv0ya.fsf@alter.siamese.dyndns.org>
On Sun, Jan 27, 2013 at 12:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> John Keeping <john@keeping.me.uk> writes:
>
>> I think I'd want to do this with a suffix if at all, so the output would
>> be like this:
>>
>> 'git mergetool --tool=<tool>' may be set to one of the following:
>>
>> araxis
>> gvimdiff
>> gvimdiff2
>> mytool (user-defined)
>> vimdiff
>> vimdiff2
>
> That is fine by me, but the real users of mergetool please feel free
> to raise objections.
This seems pretty useful.
I did a bit of refactoring last night that I'd like to post here,
the end result being something that's plugged into Documentation/.
I think what I did may also help add this functionality, and
could be useful to build upon.
I'll send my patches shortly so you can take a look.
Basically, I added a simple way to loop over the tools
and filter them. This is reused in show_tool_help() and
Documentation/Makefile.
The refactoring changes how show_tool_help() works,
so I'd like you to take a look before we add a new feature
since it might make it easier to do.
--
David
^ permalink raw reply
* [PATCH 0/4] Documentation: Auto-generate merge tool lists
From: David Aguilar @ 2013-01-27 21:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
Refactor the mergetool-lib so that we can reuse it in
Documentation/Makefile. The end result is that the
diff.tool and merge.tool documentation now includes
an auto-generated list of all available tools.
This applies on top of jk/mergetool in pu.
David Aguilar (4):
mergetool--lib: Simplify command expressions
mergetool--lib: Improve the help text in guess_merge_tool()
mergetool--lib: Add functions for finding available tools
doc: Generate a list of valid merge tools
Documentation/.gitignore | 1 +
Documentation/Makefile | 16 +++++-
Documentation/diff-config.txt | 13 ++---
Documentation/merge-config.txt | 12 ++---
git-mergetool--lib.sh | 108 ++++++++++++++++++++++-------------------
5 files changed, 87 insertions(+), 63 deletions(-)
--
1.8.0.13.gf25ae33
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox