from:"Peter Wu"

[PATCH 0/3] bash-completion fixes for global git options handling

2015-10-28 Thread Peter Wu

Hi,

These patches improve bash-completion when global git options are present.
Consider this problem:

bash-4.3$ git -C ../linux staerror: invalid key: alias.../linux

This happens because the current script is unaware of the -C option. In general,
global options are not handled well (patch 001 fixes this).

Patch 2 makes the completions more aware of the --git-dir option.

Patch 3 builds on previous patches and makes completions aware of the -C option.

Kind regards,
Peter

Peter Wu (3):
  completion: ignore git options for subcommand completion
  completion: pass --git-dir to more commands
  completion: handle git -C option

 contrib/completion/git-completion.bash | 119 -
 1 file changed, 72 insertions(+), 47 deletions(-)

-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] completion: handle git -C option

2015-10-28 Thread Peter Wu

Avoid the "fatal: bad config file line 5 in config" message and properly
complete git commands having the "-C" option.

Besides the trivial command parsing, __gitdir is rewritten to apply any
directory changes requested via `git -C otherdir ...`.

Signed-off-by: Peter Wu <pe...@lekensteyn.nl>
---
 contrib/completion/git-completion.bash | 45 +++---
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index fdf0f16..1646f61 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -34,24 +34,39 @@ case "$COMP_WORDBREAKS" in
 esac
 
 # __gitdir accepts 0 or 1 arguments (i.e., location)
-# returns location of .git repo
+# outputs location of .git repo if it exists, nothing otherwise.
 __gitdir ()
 {
-   if [ -z "${1-}" ]; then
-   if [ -n "${__git_dir-}" ]; then
-   echo "$__git_dir"
-   elif [ -n "${GIT_DIR-}" ]; then
-   test -d "${GIT_DIR-}" || return 1
-   echo "$GIT_DIR"
-   elif [ -d .git ]; then
-   echo .git
+   local gitdir=${1:-}
+
+   if [ -z "$gitdir" ]; then
+   # Try the first matching --git-dir or GIT_DIR, print nothing if 
these
+   # directories are invalid.
+   for gitdir in "${__git_dir-}" "${GIT_DIR-}"; do
+   [ -n "$gitdir" ] || continue
+   if [[ "$gitdir" != /* ]]; then
+   gitdir="${__git_cd:-.}/$gitdir"
+   fi
+   if [ -d "$gitdir" ]; then
+   echo "$gitdir"
+   fi
+   return
+   done
+
+   if [ -d "${__git_cd:-.}/.git" ]; then
+   echo "${__git_cd:-.}/.git"
else
-   git rev-parse --git-dir 2>/dev/null
+   git -C "$__git_cd" rev-parse --git-dir 2>/dev/null
fi
-   elif [ -d "$1/.git" ]; then
-   echo "$1/.git"
else
-   echo "$1"
+   if [[ "$gitdir" != /* ]]; then
+   gitdir="${__git_cd:-.}/$gitdir"
+   fi
+   if [ -d "$gitdir/.git" ]; then
+   echo "$gitdir/.git"
+   elif [ -d "$gitdir" ]; then
+   echo "$gitdir"
+   fi
fi
 }
 
@@ -2566,11 +2581,12 @@ _git_whatchanged ()
 
 __git_main ()
 {
-   local i c=1 command command_word_index __git_dir __git_options
+   local i c=1 command command_word_index __git_dir __git_options 
__git_cd=.
 
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
+   -C)  ((c++)) ; __git_cd="${words[c]}" ;;
--git-dir=*) __git_dir="${i#--git-dir=}" ;;
--git-dir)   ((c++)) ; __git_dir="${words[c]}" ;;
--bare)  __git_dir="." ;;
@@ -2583,6 +2599,7 @@ __git_main ()
done
 
__git_options=(
+   -C "$__git_cd"
--git-dir="$(__gitdir)"
)
 
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] completion: pass --git-dir to more commands

2015-10-28 Thread Peter Wu

The --git-dir option influences more commands, but was not applied
during completions. For example:

# previously empty because --git-dir was not passed to ls-remote
git --git-dir=git/.git config merge.o

Add --git-dir to more git commands (but not for repo-independent
commands such as git help) and add a new internal "__git_options"
array to store this option. In future patches, the -C option will also
be added.  (Alternatively, a new wrapper function can be added instead
of duplicating `${__git_options[@]}` all over the place, but let's keep
it simple for now.)

Add a variable and comments to __git_refs for clarity. (Note that
`--git-dir` needs to be kept there because it may not be the same as
the current repo, e.g. via `git fetch /tmp/repo `.)

Signed-off-by: Peter Wu <pe...@lekensteyn.nl>
---
 contrib/completion/git-completion.bash | 52 --
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index bd9ef4c..fdf0f16 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -282,10 +282,10 @@ __gitcomp_file ()
 __git_ls_files_helper ()
 {
if [ "$2" == "--committable" ]; then
-   git -C "$1" diff-index --name-only --relative HEAD
+   git "${__git_options[@]}" -C "$1" diff-index --name-only 
--relative HEAD
else
# NOTE: $2 is not quoted in order to support multiple options
-   git -C "$1" ls-files --exclude-standard $2
+   git "${__git_options[@]}" -C "$1" ls-files --exclude-standard $2
fi 2>/dev/null
 }
 
@@ -315,7 +315,7 @@ __git_heads ()
 {
local dir="$(__gitdir)"
if [ -d "$dir" ]; then
-   git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
+   git "${__git_options[@]}" for-each-ref 
--format='%(refname:short)' \
refs/heads
return
fi
@@ -325,7 +325,7 @@ __git_tags ()
 {
local dir="$(__gitdir)"
if [ -d "$dir" ]; then
-   git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
+   git "${__git_options[@]}" for-each-ref 
--format='%(refname:short)' \
refs/tags
return
fi
@@ -336,8 +336,9 @@ __git_tags ()
 # by checkout for tracking branches
 __git_refs ()
 {
-   local i hash dir="$(__gitdir "${1-}")" track="${2-}"
+   local i hash dir="$(__gitdir "${1-}")" track="${2-}" repo
local format refs
+   # Try refs from a local repository directory (e.g. "../linux")
if [ -d "$dir" ]; then
case "$cur" in
refs|refs/*)
@@ -353,14 +354,15 @@ __git_refs ()
refs="refs/tags refs/heads refs/remotes"
;;
esac
-   git --git-dir="$dir" for-each-ref --format="%($format)" \
-   $refs
+   git "${__git_options[@]}" --git-dir="$dir" \
+   for-each-ref --format="%($format)" $refs
if [ -n "$track" ]; then
# employ the heuristic used by git checkout
# Try to find a remote branch that matches the 
completion word
# but only output if the branch name is unique
local ref entry
-   git --git-dir="$dir" for-each-ref --shell 
--format="ref=%(refname:short)" \
+   git "${__git_options[@]}" --git-dir="$dir" \
+   for-each-ref --shell 
--format="ref=%(refname:short)" \
"refs/remotes/" | \
while read -r entry; do
eval "$entry"
@@ -372,9 +374,11 @@ __git_refs ()
fi
return
fi
+   # Try refs from a remote repository by name (e.g. "origin") or a URL
+   repo="${1-}"
case "$cur" in
refs|refs/*)
-   git ls-remote "$dir" "$cur*" 2>/dev/null | \
+   git "${__git_options[@]}" ls-remote "$repo" "$cur*" 2>/dev/null 
| \
while read -r hash i; do
case "$i" in
*^{}) ;;
@@ -384,8 +388,8 @@ __git_refs ()
;;
*)
echo "HEAD"
-   git for-each-ref --format=&qu

[PATCH 1/3] completion: ignore git options for subcommand completion

2015-10-28 Thread Peter Wu

Do not assume that the first option after "git" is a subcommand. This
fixes completion such as:

# do not detect "push" as remote name
git --git-dir=git/.git push origin 
# do not overwrite "--git-dir=..." with the alias expansion
git --git-dir=git/.git gerrit-diff 

Signed-off-by: Peter Wu <pe...@lekensteyn.nl>
---
 contrib/completion/git-completion.bash | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 482ca84..bd9ef4c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,8 +531,8 @@ __git_complete_revlist ()
 
 __git_complete_remote_or_refspec ()
 {
-   local cur_="$cur" cmd="${words[1]}"
-   local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+   local cur_="$cur" cmd="${words[command_word_index]}"
+   local i c=$((command_word_index+1)) remote="" pfx="" lhs=1 
no_complete_refspec=0
if [ "$cmd" = "remote" ]; then
((c++))
fi
@@ -788,7 +788,7 @@ __git_aliased_command ()
 # __git_find_on_cmdline requires 1 argument
 __git_find_on_cmdline ()
 {
-   local word subcommand c=1
+   local word subcommand c=$command_word_index
while [ $c -lt $cword ]; do
word="${words[c]}"
for subcommand in $1; do
@@ -803,7 +803,7 @@ __git_find_on_cmdline ()
 
 __git_has_doubledash ()
 {
-   local c=1
+   local c=$command_word_index
while [ $c -lt $cword ]; do
if [ "--" = "${words[c]}" ]; then
return 0
@@ -826,8 +826,8 @@ __git_count_arguments ()
 {
local word i c=0
 
-   # Skip "git" (first argument)
-   for ((i=1; i < ${#words[@]}; i++)); do
+   # Skip "git" (first argument) and any options such as --git-dir.
+   for ((i=command_word_index; i < ${#words[@]}; i++)); do
word="${words[i]}"
 
case "$word" in
@@ -957,7 +957,7 @@ _git_bisect ()
 
 _git_branch ()
 {
-   local i c=1 only_local_ref="n" has_r="n"
+   local i c=$command_word_index only_local_ref="n" has_r="n"
 
while [ $c -lt $cword ]; do
i="${words[c]}"
@@ -992,7 +992,7 @@ _git_branch ()
 
 _git_bundle ()
 {
-   local cmd="${words[2]}"
+   local cmd="${words[command_word_index+1]}"
case "$cword" in
2)
__gitcomp "create list-heads verify unbundle"
@@ -1760,7 +1760,7 @@ _git_stage ()
 __git_config_get_set_variables ()
 {
local prevword word config_file= c=$cword
-   while [ $c -gt 1 ]; do
+   while [ $c -gt $command_word_index ]; do
word="${words[c]}"
case "$word" in
--system|--global|--local|--file=*)
@@ -2516,7 +2516,7 @@ _git_svn ()
 
 _git_tag ()
 {
-   local i c=1 f=0
+   local i c=$command_word_index f=0
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
@@ -2562,7 +2562,7 @@ _git_whatchanged ()
 
 __git_main ()
 {
-   local i c=1 command __git_dir
+   local i c=1 command command_word_index __git_dir
 
while [ $c -lt $cword ]; do
i="${words[c]}"
@@ -2573,7 +2573,7 @@ __git_main ()
--help) command="help"; break ;;
-c|--work-tree|--namespace) ((c++)) ;;
-*) ;;
-   *) command="$i"; break ;;
+   *) command="$i"; command_word_index=$c; break ;;
esac
((c++))
done
@@ -2608,7 +2608,7 @@ __git_main ()
 
local expansion=$(__git_aliased_command "$command")
if [ -n "$expansion" ]; then
-   words[1]=$expansion
+   words[command_word_index]=$expansion
completion_func="_git_${expansion//-/_}"
declare -f $completion_func >/dev/null && $completion_func
fi
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git compile warnings (under mac/clang)

2015-01-22 Thread Peter Wu

On Thursday 22 January 2015 11:59:54 Stefan Beller wrote:
 cc Johannes Schindelin johannes.schinde...@gmx.de who is working in
 the fsck at the moment
 cc Peter Wu pe...@lekensteyn.nl who worked on builtin/remote.c a few weeks 
 ago
 
 I just compiled origin/pu to test and also found a problem (doesn't
 happen in origin/master):
 
 http.c: In function 'get_preferred_languages':
 http.c:1020:2: warning: implicit declaration of function 'setlocale'
 [-Wimplicit-function-declaration]
   retval = setlocale(LC_MESSAGES, NULL);
   ^
 http.c:1020:21: error: 'LC_MESSAGES' undeclared (first use in this function)
   retval = setlocale(LC_MESSAGES, NULL);
  ^
 http.c:1020:21: note: each undeclared identifier is reported only once
 for each function it appears in
 
 so I cc Yi EungJun eungjun...@navercorp.com as well.
 
 On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume blume.m...@gmail.com wrote:
  These are probably minor, I only bring them up because Git's build is
  generally so quiet that it might be worth squashing these too.
 
  CC fsck.o
  fsck.c:110:38: warning: comparison of unsigned enum expression = 0 is
  always true [-Wtautological-compare]
  if (options-msg_severity  msg_id = 0  msg_id  FSCK_MSG_MAX)
   ~~ ^  ~
  1 warning generated.
  AR libgit.a
  /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib:
  file: libgit.a(gettext.o) has no symbols
  CC builtin/remote.o
  builtin/remote.c:1572:5: warning: add explicit braces to avoid
  dangling else [-Wdangling-else]
  else
  ^
  builtin/remote.c:1580:5: warning: add explicit braces to avoid
  dangling else [-Wdangling-else]
  else
  ^
  2 warnings generated.

Hi, these warnings were present in v3 of the git-remote patch. v4 was
proposed to overcome these issues, but I have yet to respond to Junio's
feedback at http://www.spinics.net/lists/git/msg243652.html
(Message-ID: xmqqr3vx9ad5@gitster.dls.corp.google.com)

(cc'ing Junio to let him know I am still alive :p)

I'll get back to this next week, had some other tasks to prepare for.

Kind regards,
Peter

  (the warning about libgit.a(gettext.o) is probably because I'm
  building with NO_GETTEXT -- I've never been able to get gettext to
  work on my mac)

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] remote: add --fetch and --both options to set-url

2014-12-17 Thread Peter Wu

git remote set-url knew about the '--push' option to update just the
pushurl, but it does not have a similar option for update fetch URL and
leave whatever was in place for the push URL.

This patch adds support for a '--fetch' option which implements that use
case in a backwards compatible way: if no --both, --push or --fetch
options are given, then the push URL is modified too if it was not set
before. This is the case since the push URL is implicitly based on the
fetch URL.

A '--both' option is added to make the command independent of previous
pushurl settings. For the --add and --delete set operations, it will
always set the push and/ or the fetch URLs. For the primary mode of
operation (without --add or --delete), it will drop pushurl as the
implicit push URL is the (fetch) URL.

While '--both' could be implemented as '--fetch --push', it might also
be mistaken for --push overrides --fetch. An option such as
--only={fetch|push|both} was also considered, but it is longer than
the current options, makes --push redundant and brings the confusing
option --only=both. Options such as '--direction=...' is too long and
'--dir=' is ambiguous (directory?). Thus, for brevity three new
options were introduced.

The documentation has also been updated and a missing '--push' option
is added to the 'git remote -h' command.

Tests are also added to verify the documented behavior.

Signed-off-by: Peter Wu pe...@lekensteyn.nl
---
Hi,

This is the fourth revision of the patch that allows for just setting the fetch
URL. Eric wondered why '--fetch --push' is not used to describe the state
'--both', so I added this to the commit message.

The t5505-remote.sh test still passes after this change (I was unable to run the
full test suite as it broke due to an unrelated gpg issue).

Kind regards,
Peter

 v2: fixed test case
 v3: added --both option, changed --fetch --push behavior, added more tests for
 --add/--delete cases, refactored to reduce duplication (not special-casing
 add_mode without oldurl, just skip initially setting oldurl).
 v4: incorporated documentation suggestion from Eric Sunshine;
 Tried to make the code easier to follow by replacing
 (modify_type == MODIFY_TYPE_FETCH) by
 (modify_type  MODIFY_TYPE_FETCH  modify_type != MODIFY_TYPE_HISTORIC)
 and added comments explaining why this is special (observed by Jeff King);
 Fixed dangling else issue reported by Torsten Bögershausen;
 Added considerations for other options to the commit message;
 Rebased on v2.2.0-65-g9abc44b.
---

 Documentation/git-remote.txt |  16 +++--
 builtin/remote.c | 146 ---
 t/t5505-remote.sh| 125 
 3 files changed, 234 insertions(+), 53 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index cb103c8..09a4670 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,9 +15,9 @@ SYNOPSIS
 'git remote remove' name
 'git remote set-head' name (-a | --auto | -d | --delete | branch)
 'git remote set-branches' [--add] name branch...
-'git remote set-url' [--push] name newurl [oldurl]
-'git remote set-url --add' [--push] name newurl
-'git remote set-url --delete' [--push] name url
+'git remote set-url' [--both | --fetch | --push] name newurl [oldurl]
+'git remote set-url' [--both | --fetch | --push] '--add' name newurl
+'git remote set-url' [--both | --fetch | --push] '--delete' name url
 'git remote' [-v | --verbose] 'show' [-n] name...
 'git remote prune' [-n | --dry-run] name...
 'git remote' [-v | --verbose] 'update' [-p | --prune] [(group | remote)...]
@@ -134,7 +134,15 @@ Changes URL remote points to. Sets first URL remote points 
to matching
 regex oldurl (first URL if no oldurl is given) to newurl. If
 oldurl doesn't match any URL, error occurs and nothing is changed.
 +
-With '--push', push URLs are manipulated instead of fetch URLs.
+With '--both', both the fetch and push URLs are manipulated.
++
+With '--fetch', only fetch URLs are manipulated.
++
+With '--push', only push URLs are manipulated.
++
+For historical reasons, if neither --fetch nor --push is specified then the
+fetch URL is changed, as well as the push URL if this was not already set. This
+behavior may change in the future.
 +
 With '--add', instead of changing some URL, new URL is added.
 +
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..1fa2fd7 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
N_(git remote prune [-n | --dry-run] name),
N_(git remote [-v | --verbose] update [-p | --prune] [(group | 
remote)...]),
N_(git remote set-branches [--add] name branch...),
-   N_(git remote set-url [--push] name newurl [oldurl]),
-   N_(git remote set-url --add name newurl),
-   N_(git remote set-url --delete name url),
+   N_(git remote set-url [--both

Re: [PATCH v3] remote: add --fetch and --both options to set-url

2014-12-17 Thread Peter Wu

On Wednesday 17 December 2014 11:08:07 Torsten Bögershausen wrote:
 On 11/25/2014 12:48 PM, Peter Wu wrote:
  git remote set-url knew about the '--push' option to update just the
  pushurl, but it does not have a similar option for update fetch URL and
  leave whatever was in place for the push URL.
 
  This patch adds support for a '--fetch' option which implements that use
  case in a backwards compatible way: if no --both, --push or --fetch
  options are given, then the push URL is modified too if it was not set
  before. This is the case since the push URL is implicitly based on the
  fetch URL.
 
  A '--both' option is added to make the command independent of previous
  pushurl settings. For the --add and --delete set operations, it will
  always set the push and/ or the fetch URLs. For the primary mode of
  operation (without --add or --delete), it will drop pushurl as the
  implicit push URL is the (fetch) URL.
 
  The documentation has also been updated and a missing '--push' option
  is added to the 'git remote -h' command.
 
  Tests are also added to verify the documented behavior.
 
  Signed-off-by: Peter Wu pe...@lekensteyn.nl
  ---
 
v2: fixed test case
v3: added --both option, changed --fetch --push behavior, added more 
  tests for
--add/--delete cases, refactored to reduce duplication (not 
  special-casing
add_mode without oldurl, just skip initially setting oldurl).
 
  Translators note: the help text gained more translatable strings and some
  strings got additional options.
  ---
Documentation/git-remote.txt |  17 --
builtin/remote.c | 140 
  +++
t/t5505-remote.sh| 125 ++
3 files changed, 228 insertions(+), 54 deletions(-)
 
  diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
  index cb103c8..bdd0305 100644
  --- a/Documentation/git-remote.txt
  +++ b/Documentation/git-remote.txt
  @@ -15,9 +15,9 @@ SYNOPSIS
'git remote remove' name
'git remote set-head' name (-a | --auto | -d | --delete | branch)
'git remote set-branches' [--add] name branch...
  -'git remote set-url' [--push] name newurl [oldurl]
  -'git remote set-url --add' [--push] name newurl
  -'git remote set-url --delete' [--push] name url
  +'git remote set-url' [--both | --fetch | --push] name newurl [oldurl]
  +'git remote set-url' [--both | --fetch | --push] '--add' name newurl
  +'git remote set-url' [--both | --fetch | --push] '--delete' name url
'git remote' [-v | --verbose] 'show' [-n] name...
'git remote prune' [-n | --dry-run] name...
'git remote' [-v | --verbose] 'update' [-p | --prune] [(group | 
  remote)...]
  @@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote 
  points to matching
regex oldurl (first URL if no oldurl is given) to newurl. If
oldurl doesn't match any URL, error occurs and nothing is changed.
+
  -With '--push', push URLs are manipulated instead of fetch URLs.
  +With '--both', both the fetch and push URLs are manipulated.
  ++
  +With '--fetch', only fetch URLs are manipulated.
  ++
  +With '--push', only push URLs are manipulated.
  ++
  +If none of the '--both', '--fetch' or --push' options are given, then
  +'--both' applies only if no push URL was set before. Otherwise '--fetch'
  +is assumed for historical reasons. This default may change in the
  +future to '--both' to avoid surprises depending on the configuration.
+
With '--add', instead of changing some URL, new URL is added.
+
  diff --git a/builtin/remote.c b/builtin/remote.c
  index 7f28f92..a250b23 100644
  --- a/builtin/remote.c
  +++ b/builtin/remote.c
  @@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
  N_(git remote prune [-n | --dry-run] name),
  N_(git remote [-v | --verbose] update [-p | --prune] [(group | 
  remote)...]),
  N_(git remote set-branches [--add] name branch...),
  -   N_(git remote set-url [--push] name newurl [oldurl]),
  -   N_(git remote set-url --add name newurl),
  -   N_(git remote set-url --delete name url),
  +   N_(git remote set-url [--both | --fetch | --push] name newurl 
  [oldurl]),
  +   N_(git remote set-url [--both | --fetch | --push] --add name 
  newurl),
  +   N_(git remote set-url [--both | --fetch | --push] --delete name 
  url),
  NULL
};

  @@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = 
  {
};

static const char * const builtin_remote_seturl_usage[] = {
  -   N_(git remote set-url [--push] name newurl [oldurl]),
  -   N_(git remote set-url --add name newurl),
  -   N_(git remote set-url --delete name url),
  +   N_(git remote set-url [--both | --fetch | --push] name newurl 
  [oldurl]),
  +   N_(git remote set-url [--both | --fetch | --push] --add name 
  newurl),
  +   N_(git remote set-url [--both | --fetch | --push] --delete name 
  url),
  NULL

Re: [PATCH v4] remote: add --fetch and --both options to set-url

2014-12-17 Thread Peter Wu

On Wednesday 17 December 2014 09:32:13 Jeff King wrote:
 On Wed, Dec 17, 2014 at 03:18:56PM +0100, Peter Wu wrote:
 
  This is the fourth revision of the patch that allows for just setting the 
  fetch
  URL. Eric wondered why '--fetch --push' is not used to describe the state
  '--both', so I added this to the commit message.
 
 Thanks, I think that explanation is very clear.
 
 This version overall looks good to me.
 
  The t5505-remote.sh test still passes after this change (I was unable to 
  run the
  full test suite as it broke due to an unrelated gpg issue).
 
 It is it because you have gpg 2.1, and the patches to handle that are
 not yet merged to master? Or is it because you are using the new patches
 and they are breaking things for your older gpg version?
 
 If the former, that's fine. If the latter, it would be nice to see a
 report of the breakage. :)

Rest assured, this is a gpg 2.1 issue :-)
-- 
Kind regards,
Peter
https://lekensteyn.nl

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-12-02 Thread Peter Wu

On Saturday 29 November 2014 13:31:18 Philip Oakley wrote:
 From: Peter Wu pe...@lekensteyn.nl
  Ok, I will make a clear note about the default (without --only) 
  behavior
  having weird behavior for historical reasons. Are you really OK with
  --only=both? It sounds a bit odd (mathematically speaking it is 
  correct
  as fetch and push are both partitions that form the whole set if you
  ignore the historical behavior).
 
 How about :
 
 s/--only/--direction/
 
 or some suitable abbreviation (--dirn ?)

In the next version of the patch I went for three separate options,
--fetch, --push and --both:
http://article.gmane.org/gmane.comp.version-control.git/260213
(Juno, Jeff: ping?)

The option --direction=fetch|push|both is a bit longer and --dirn can
be mistaken for directory N.
-- 
Kind regards,
Peter
https://lekensteyn.nl

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-25 Thread Peter Wu

On Monday 24 November 2014 23:08:26 Jeff King wrote:
 However, I think what removed the confusion for me in your --only=both
 proposal was the presence of a both option, since it made it more
 clear that is not what no-option means. So what about just --push,
 --fetch, and --both? Explain the current behavior of no-options in
 the documentation as a historical oddity.

Ok, this sounds even better. I have dropped the --only part and made the
options --push, --fetch and --both disjoint (overriding each other). A
patch will follow soon. Maybe it should warn when you try to specify
both options though.

 That also gives us an easy path forward for changing the behavior.
 During the transition period, people should use --push, --fetch, or
 --both. Using no-options provides a warning. After a settling period,
 the no-option behavior will switch to one of those (presumably --both),
 and drop the warning.
 
 You do not have to do the migration path if you don't want to. Adding
 --fetch and --both scratches your itch and sets us up to migrate
 later.

I have documented the historic behavior and mentioned that it is
/possible/ that the option --both becomes default in the future.

  What about the translations? Should I send a separate patch for that or
  can I update all translations at once?
 
 You do not have to update the translations. When we near a release, the
 l10n coordinator will run make pot to update po/git.pot with the
 strings marked for translation, and then the translators will write
 translations for the new strings. You are of course welcome to help with
 the translation effort at that stage. 
 
 Details are in po/README.

Well, it is not necessary the translations, but the format of them. The
format
git remote set-url --delete name url has changed to
git remote set-url [--both | --fetch | --push] --delete name url
for example. The old strings are still usable, so I wonder whether I can
make it easier for the i10n maintainer to recognize this change?
-- 
Kind regards,
Peter
https://lekensteyn.nl

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-25 Thread Peter Wu

On Monday 24 November 2014 21:19:16 Junio C Hamano wrote:
 On Mon, Nov 24, 2014 at 9:01 PM, Jeff King p...@peff.net wrote:
  We could also stop making push fall back to fetch. But I think people
  would find that irritating.

The common case is probably having the same fetch and push URL, so I
think that this should not be changed.

  I dunno. I think there has always been an implicit subordinate
  relationship between fetch and push URLs, with fetch being the main
  one. Maybe that is so ingrained in me at this point that I do not see a
  problem with the asymmetry.
 
 I actually do not have problem with asymmetry/subordinate
 relationship myself, but then why are we adding --fetch to
 complement --push in the first place?
 
 Or perhaps I am misunderstanding the suggested semantics
 of --both. That subordinate relationship really means that
 remote.nick.URL is the one that is used for both directions
 when pushURL is not set.
 
 I misunderstood that --both would add identical value to both
 remote.nick.URL and remote.nick.pushURL, but that would
 break the implicit subordinate relationship. Is the suggested
 semantics of set-url --both to first delete remote.nick.pushURL
 if exists and then to set remote.nick.URL?
 
 If that is what is being proposed, then I think it makes sense.

Yes, your last understanding is correct. For this feature, try to think
as the user who does not know about the configuration implementation.
That is why the --fetch option was proposed, earlier it did not make
sense to me why a --push option exists, but a --fetch option is missing.

Option '--both' will drop the push URL and result in an implicit
fallback to the fetch URL. It becomes slightly more hairy in the
presence of URL sets (using --add and --delete), but I have also tried
to make that act sensibly).
-- 
Kind regards,
Peter
https://lekensteyn.nl

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] remote: add --fetch and --both options to set-url

2014-11-25 Thread Peter Wu

git remote set-url knew about the '--push' option to update just the
pushurl, but it does not have a similar option for update fetch URL and
leave whatever was in place for the push URL.

This patch adds support for a '--fetch' option which implements that use
case in a backwards compatible way: if no --both, --push or --fetch
options are given, then the push URL is modified too if it was not set
before. This is the case since the push URL is implicitly based on the
fetch URL.

A '--both' option is added to make the command independent of previous
pushurl settings. For the --add and --delete set operations, it will
always set the push and/ or the fetch URLs. For the primary mode of
operation (without --add or --delete), it will drop pushurl as the
implicit push URL is the (fetch) URL.

The documentation has also been updated and a missing '--push' option
is added to the 'git remote -h' command.

Tests are also added to verify the documented behavior.

Signed-off-by: Peter Wu pe...@lekensteyn.nl
---

 v2: fixed test case
 v3: added --both option, changed --fetch --push behavior, added more tests for
 --add/--delete cases, refactored to reduce duplication (not special-casing
 add_mode without oldurl, just skip initially setting oldurl).

Translators note: the help text gained more translatable strings and some
strings got additional options.
---
 Documentation/git-remote.txt |  17 --
 builtin/remote.c | 140 +++
 t/t5505-remote.sh| 125 ++
 3 files changed, 228 insertions(+), 54 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index cb103c8..bdd0305 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,9 +15,9 @@ SYNOPSIS
 'git remote remove' name
 'git remote set-head' name (-a | --auto | -d | --delete | branch)
 'git remote set-branches' [--add] name branch...
-'git remote set-url' [--push] name newurl [oldurl]
-'git remote set-url --add' [--push] name newurl
-'git remote set-url --delete' [--push] name url
+'git remote set-url' [--both | --fetch | --push] name newurl [oldurl]
+'git remote set-url' [--both | --fetch | --push] '--add' name newurl
+'git remote set-url' [--both | --fetch | --push] '--delete' name url
 'git remote' [-v | --verbose] 'show' [-n] name...
 'git remote prune' [-n | --dry-run] name...
 'git remote' [-v | --verbose] 'update' [-p | --prune] [(group | remote)...]
@@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote points 
to matching
 regex oldurl (first URL if no oldurl is given) to newurl. If
 oldurl doesn't match any URL, error occurs and nothing is changed.
 +
-With '--push', push URLs are manipulated instead of fetch URLs.
+With '--both', both the fetch and push URLs are manipulated.
++
+With '--fetch', only fetch URLs are manipulated.
++
+With '--push', only push URLs are manipulated.
++
+If none of the '--both', '--fetch' or --push' options are given, then
+'--both' applies only if no push URL was set before. Otherwise '--fetch'
+is assumed for historical reasons. This default may change in the
+future to '--both' to avoid surprises depending on the configuration.
 +
 With '--add', instead of changing some URL, new URL is added.
 +
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..a250b23 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
N_(git remote prune [-n | --dry-run] name),
N_(git remote [-v | --verbose] update [-p | --prune] [(group | 
remote)...]),
N_(git remote set-branches [--add] name branch...),
-   N_(git remote set-url [--push] name newurl [oldurl]),
-   N_(git remote set-url --add name newurl),
-   N_(git remote set-url --delete name url),
+   N_(git remote set-url [--both | --fetch | --push] name newurl 
[oldurl]),
+   N_(git remote set-url [--both | --fetch | --push] --add name 
newurl),
+   N_(git remote set-url [--both | --fetch | --push] --delete name 
url),
NULL
 };
 
@@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = {
 };
 
 static const char * const builtin_remote_seturl_usage[] = {
-   N_(git remote set-url [--push] name newurl [oldurl]),
-   N_(git remote set-url --add name newurl),
-   N_(git remote set-url --delete name url),
+   N_(git remote set-url [--both | --fetch | --push] name newurl 
[oldurl]),
+   N_(git remote set-url [--both | --fetch | --push] --add name 
newurl),
+   N_(git remote set-url [--both | --fetch | --push] --delete name 
url),
NULL
 };
 
@@ -1503,21 +1503,35 @@ static int set_branches(int argc, const char **argv)
return set_remote_branches(argv[0], argv + 1, add_mode);
 }
 
+#define MODIFY_TYPE_FETCH   (1  0)
+#define MODIFY_TYPE_PUSH(1  1)
+#define MODIFY_TYPE_BOTH(MODIFY_TYPE_FETCH | MODIFY_TYPE_PUSH

Re: [PATCH v3] remote: add --fetch and --both options to set-url

2014-11-25 Thread Peter Wu

On Tuesday 25 November 2014 17:09:04 Eric Sunshine wrote:
 On Tue, Nov 25, 2014 at 6:48 AM, Peter Wu pe...@lekensteyn.nl wrote:
  git remote set-url knew about the '--push' option to update just the
  pushurl, but it does not have a similar option for update fetch URL and
  leave whatever was in place for the push URL.
 
  This patch adds support for a '--fetch' option which implements that use
  case in a backwards compatible way: if no --both, --push or --fetch
  options are given, then the push URL is modified too if it was not set
  before. This is the case since the push URL is implicitly based on the
  fetch URL.
 
  A '--both' option is added to make the command independent of previous
  pushurl settings. For the --add and --delete set operations, it will
  always set the push and/ or the fetch URLs. For the primary mode of
  operation (without --add or --delete), it will drop pushurl as the
  implicit push URL is the (fetch) URL.
 
 I've read (though perhaps not fully digested) the other email thread
 which led up to this version of the patch, as well as the
 documentation update in this patch, but I still don't understand the
 need for the --both option. Intuitively, I would expect that
 specifying --fetch and --push on the command line would have the same
 effect as the proposed --both option. Thus, why is a separate (and
 exclusive) --both option needed? Is it to reduce typing? What am I
 missing?

The initial version just added --fetch and let --fetch --push behave
like the current --both. Here you can see the most recent discussion
leading to the --both option:
http://www.spinics.net/lists/git/msg242336.html

There was an overlapping discussion about the confusing historic
behavior (default vs. --fetch vs. --push --fetch), and an alternative
(less verbose form) of --push --fetch. The reason that --both is
introduced probably has something to do with it being less verbose.
I am not too attached to either option by the way.

  The documentation has also been updated and a missing '--push' option
  is added to the 'git remote -h' command.
 
  Tests are also added to verify the documented behavior.
 
  Signed-off-by: Peter Wu pe...@lekensteyn.nl
  ---
 
   v2: fixed test case
   v3: added --both option, changed --fetch --push behavior, added more tests 
  for
   --add/--delete cases, refactored to reduce duplication (not 
  special-casing
   add_mode without oldurl, just skip initially setting oldurl).
 
  Translators note: the help text gained more translatable strings and some
  strings got additional options.
  ---
  diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
  index cb103c8..bdd0305 100644
  --- a/Documentation/git-remote.txt
  +++ b/Documentation/git-remote.txt
  @@ -134,7 +134,16 @@ Changes URL remote points to. Sets first URL remote 
  points to matching
   regex oldurl (first URL if no oldurl is given) to newurl. If
   oldurl doesn't match any URL, error occurs and nothing is changed.
   +
  -With '--push', push URLs are manipulated instead of fetch URLs.
  +With '--both', both the fetch and push URLs are manipulated.
  ++
  +With '--fetch', only fetch URLs are manipulated.
  ++
  +With '--push', only push URLs are manipulated.
  ++
  +If none of the '--both', '--fetch' or --push' options are given, then
  +'--both' applies only if no push URL was set before. Otherwise '--fetch'
  +is assumed for historical reasons. This default may change in the
  +future to '--both' to avoid surprises depending on the configuration.
 
 This explanation is somewhat tortuous. Assuming that the --both option
 is superfluous, perhaps this could be explained more clearly along
 these lines:
 
 --- 8 ---
 `--fetch` changes fetch URLs, and --push changes push URLs. Specified
 together, both fetch and push URLs are changed.
 
 For historical reasons, if neither --fetch nor --push is specified,
 then the fetch URL is changed, as well as the push URL if not already
 set.
 --- 8 ---

Excellent, short and concise. If this path is taken, I would still add
the note about the future change.

I am also interested in opinions about the suggested default behavior.
Should it become --both (--push --fetch)? In that case '--both' will be
a pretty useless option in the future (when it becomes the default).

   +
   With '--add', instead of changing some URL, new URL is added.
   +

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-24 Thread Peter Wu

ping?

I asked around and the people who know of `git remote` fell in these two
categories:

 - those who know of this bug and then first set the fetch URL and
   then the push URL.
 - those who did not expect the current behavior.

The command 'git remote set-url NAME URL' reads as set the URL(s) for
remote NAME to URL. Currently it means set the fetch (and push) URL of
remote NAME to URL (depending on whether pushurl is set).

I propose to add the option --fetch next to --push with the meaning set
the fetch/push URL of remote NAME to URL. Then --fetch --push means
set the fetch and push URL of remote NAME to URL. In a future git
version, this could be made the default option to avoid surprises (which
would be backwards incompatible though).

As for the changelog entry,

The git remote set-url command now allows you to change just the
fetch URL without modifying the push URL using the new --fetch
option. For symmetry with the --push option.

(symmetry in the eyes of the user, not how it is implemented in the
git config.)

Opinions?

On Wednesday 19 November 2014 22:28:35 Peter Wu wrote:
 On Wednesday 19 November 2014 13:18:56 Junio C Hamano wrote:
  Junio C Hamano gits...@pobox.com writes:
   Jeff King p...@peff.net writes:
   If you are fetching from somebody else and then pushing into your
   own publishing repository (i.e. fork of that upstream), why isn't
   the sequence of event like this, instead?
  
   $ git clone $upstream
   $ browser github.com
   ... fork upstream to your own publishing repository ...
   $ git remote set-url --push mine url for your publish repo
  
   Isn't this one of those bad workflows encouraged by GitHub, for
   which you guys have to be punished ;-)?
 
 For forks, it usually goes like this:
 
 git clone $upstream
 ... realizes that is has a bug which I want to fix ...
 ... creates a new repo ...
 git remote rename origin upstream
 git remote add origin git@$personal_repo
 # --fetch is what I need
 git remote add --fetch https://$personal_repo
 
 I often start by entering/copying the ssh URL which is what I need for
 pushing. Later ssh-agent forget about my key and I realize that push
 works fine over https, so would like to set that... only to observe that
 is not possible in an straightforward way through 'git remote'.
 
  Coming back to the topic, how common would this oops, I cloned via
  a wrong transport be?  I am not opposed to giving a recovery method
  for gotcha that does not happen very often, but if such an addition
  adds undue confusion factor for people who use set-url for more
  common cases, that would be a bad trade-off.
 
 Well, people rarely need to use 'git remote' except when, well, they
 need to modify the remotes. Where does the confusion come from? I might
 be biased now that I know the internals. Maybe the https/ssh case above
 needs to be mentioned in the documentation? What do you think of the
 updated documentation by the way?

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-24 Thread Peter Wu

On Monday 24 November 2014 14:04:07 Junio C Hamano wrote:
 Peter Wu pe...@lekensteyn.nl writes:
 
  I propose to add the option --fetch next to --push with the meaning set
  the fetch/push URL of remote NAME to URL. Then --fetch --push means
  set the fetch and push URL of remote NAME to URL. 
 
 What would (and should) the configuration look like after you did
 this?
 
   git remote set-url nick $url1
 git remote set-url nick --push $url2
 git remote set-url nick $url3
 
 Whatever happens without your patch after the above is what the
 current users (i.e. those who do not use the --fetch option) expect,
 so if the behaviour does not change with your patch, then there is
 one less incompatibilities to worry about.
 
 A new option --fetch introducing a different behaviour is
 perfectly fine; existing users who are not using it will not be
 harmed by sudden behaviour change.

As stated before, I took care to avoid backwards incompatibilities. The
command will still work as expected by the users who are aware of this
particular behavior. What I am suggesting (and which is independent of
the patch) is to make the command have a more consistent behavior.
Either it should set the fetch URL, or both the fetch and push URL, but
not vary its behavior depending on whether a push URL is set or not.
That should make the behavior of the command more consistent.

  In a future git version, this could be made the default option to
  avoid surprises (which would be backwards incompatible though).
 
 I am not sure what you mean by default.  If you mean set both if
 remote.nick.pushurl does not exist but otherwise update only
 remote.nick.url, then the sequence
 
   git remote set-url nick $url1
 git remote set-url nick --push $url2
 git remote set-url nick $url3
 
 would retain the current behaviour, so it probably is OK.
 
 If you mean to always set remote.nick.url and remote.nick.pushurl
 pointing at the same value when neither --fetch nor --push is given,
 That would make the sequence behave quite different from what people
 would expect, and you would need to devise a transition plan to
 first start warning when the user did something that will behave
 differently between the current version and the future version
 without changing the behaviour, then switch the behaviour but keep
 warning and finally remove the warning, or something like that.
 
 And the above three-command sequence may not be the only case where
 the change you are proposing may hurt existing users.

The default refers to the behavior of git remote set-url in absence
of --push and --fetch options. A transition period is expected (if
this idea is put forward). Since nobody seems to be bitten by this
option, I am not sure if it really adds much value to make this change
though.
-- 
Kind regards,
Peter
https://lekensteyn.nl

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-24 Thread Peter Wu

On Monday 24 November 2014 17:22:44 Jeff King wrote:
 On Mon, Nov 24, 2014 at 11:16:03PM +0100, Peter Wu wrote:
 
   A new option --fetch introducing a different behaviour is
   perfectly fine; existing users who are not using it will not be
   harmed by sudden behaviour change.
  
  As stated before, I took care to avoid backwards incompatibilities. The
  command will still work as expected by the users who are aware of this
  particular behavior.
 
 Right. My original complaint was only that --fetch is not as
 orthogonal to --push (and an optionless set-url) as it could be. I
 think the alternatives for going forward are basically:
 
   1. Name it something besides --fetch (but that's rather clunky).

It is not orthogonal to --push in the config, but the behavior exposed
to the user is orthogonal unless I am missing something?

I can understand that --fetch sounds a bit weird, what about this
natural translation:

git remote: set the URL (only the fetch one) for NAME to URL
git remote set-url --only=fetch NAME URL

git remote: set the URL (only the push one) for NAME to URL
git remote set-url --only=push NAME URL
(obsoletes --push)

git remote: set the URL (both) for NAME to URL
git remote set-url --only=both NAME URL
(it would be nice if --only=both (weird!) can be removed in the
future such that the option is more natural)

git remote: set the URL for NAME to URL
git remote set-url NAME URL
(current behavior: YOU git guru knows what I do right?)


   2. Migrate to new behavior, which is what is being discussed here.
  Probably needs a transition period?

A transition period would also help to solicit feedback.

   3. Live with it. Probably address the weirdness in the documentation.
 
   4. Do nothing, drop the patch.
 
 I think I'd be OK with (3), with an appropriate documentation update.

I prefer 1 for now as it avoids the extra manual action I have to take
when changing URLs.

Peter

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-24 Thread Peter Wu

On Monday 24 November 2014 17:54:57 Jeff King wrote:
 On Mon, Nov 24, 2014 at 11:47:30PM +0100, Peter Wu wrote:
  I can understand that --fetch sounds a bit weird, what about this
  natural translation:
  
  git remote: set the URL (only the fetch one) for NAME to URL
  git remote set-url --only=fetch NAME URL
  
  git remote: set the URL (only the push one) for NAME to URL
  git remote set-url --only=push NAME URL
  (obsoletes --push)
  
  git remote: set the URL (both) for NAME to URL
  git remote set-url --only=both NAME URL
  (it would be nice if --only=both (weird!) can be removed in the
  future such that the option is more natural)
  
  git remote: set the URL for NAME to URL
  git remote set-url NAME URL
  (current behavior: YOU git guru knows what I do right?)
 
 Yeah, I think that addresses my concern (because it explicitly leaves
 no-option as a historical curiosity, and not as an implicit version of
 --both).

Ok, I will make a clear note about the default (without --only) behavior
having weird behavior for historical reasons. Are you really OK with
--only=both? It sounds a bit odd (mathematically speaking it is correct
as fetch and push are both partitions that form the whole set if you
ignore the historical behavior).

 3. Live with it. Probably address the weirdness in the documentation.
   
 4. Do nothing, drop the patch.
   
   I think I'd be OK with (3), with an appropriate documentation update.
  
  I prefer 1 for now as it avoids the extra manual action I have to take
  when changing URLs.
 
 I'm not sure if I was clear on (3), but live with it was live with
 your original patch. Which I think you would also be happy with.

Oh yes, I misunderstood this one ;)

What about the translations? Should I send a separate patch for that or
can I update all translations at once?
-- 
Kind regards,
Peter
https://lekensteyn.nl

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-19 Thread Peter Wu

git remote set-url knew about the '--push' option to update just the
pushurl, but it does not have a similar option for update fetch URL and
leave whatever was in place for the push URL.

This patch adds support for a '--fetch' option which implements that use
case in a backwards compatible way: if neither --push nor --fetch are
given, then the push URL is modified too if it was not set before. This
is the case since the push URL is implicitly based on the fetch URL.

The documentation has also been updated and a missing '--push' option
is added to the 'git remote -h' command.

Tests are also added to verify the documented behavior.

Signed-off-by: Peter Wu pe...@lekensteyn.nl
---
(please cc me, I am not subscribed)

Hi,

Now and then I hit this issue where I want to change the remote where
sources are fetched from without having to touch the push URL. Example:

git remote add gh g...@github.com:Lekensteyn/git.git
# Avoid needing SSH for pulling from a repo, so change fetch URL
git remote set-url https://github.com/Lekensteyn/git.git
# Hmm, the fetch URL got changed too, let's fix that.
git remote add --push gh g...@github.com:Lekensteyn/git.git

After this patch, I can use:

git remote add gh g...@github.com:Lekensteyn/git.git
git remote set-url --fetch https://github.com/Lekensteyn/git.git

rather than being forced into a specific order:

git remote add gh https://github.com/Lekensteyn/git.git
git remote set-url --push gh g...@github.com:Lekensteyn/git.git

The functionality is implemented, but it might need a refactoring to
reduce duplicate code. Translation strings also need to be updated (the
current 'git remote set-url' strings are also not up to date).

Kind regards.
Peter
---
 Documentation/git-remote.txt |  12 +++--
 builtin/remote.c | 106 +--
 t/t5505-remote.sh|  53 ++
 3 files changed, 133 insertions(+), 38 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index cb103c8..7bb21a2 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,9 +15,9 @@ SYNOPSIS
 'git remote remove' name
 'git remote set-head' name (-a | --auto | -d | --delete | branch)
 'git remote set-branches' [--add] name branch...
-'git remote set-url' [--push] name newurl [oldurl]
-'git remote set-url --add' [--push] name newurl
-'git remote set-url --delete' [--push] name url
+'git remote set-url' [--fetch] [--push] name newurl [oldurl]
+'git remote set-url' [--fetch] [--push] '--add' name newurl
+'git remote set-url' [--fetch] [--push] '--delete' name url
 'git remote' [-v | --verbose] 'show' [-n] name...
 'git remote prune' [-n | --dry-run] name...
 'git remote' [-v | --verbose] 'update' [-p | --prune] [(group | remote)...]
@@ -134,7 +134,11 @@ Changes URL remote points to. Sets first URL remote points 
to matching
 regex oldurl (first URL if no oldurl is given) to newurl. If
 oldurl doesn't match any URL, error occurs and nothing is changed.
 +
-With '--push', push URLs are manipulated instead of fetch URLs.
+With '--push', push URLs are manipulated.
++
+With '--fetch', fetch URLs are manipulated. If neither '--push' nor '--fetch'
+are given, then (1) '--fetch' is assumed if no push URL is set in the
+configuration or (2) '--fetch --push' otherwise.
 +
 With '--add', instead of changing some URL, new URL is added.
 +
diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..26caa6f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,9 +18,9 @@ static const char * const builtin_remote_usage[] = {
N_(git remote prune [-n | --dry-run] name),
N_(git remote [-v | --verbose] update [-p | --prune] [(group | 
remote)...]),
N_(git remote set-branches [--add] name branch...),
-   N_(git remote set-url [--push] name newurl [oldurl]),
-   N_(git remote set-url --add name newurl),
-   N_(git remote set-url --delete name url),
+   N_(git remote set-url [--fetch] [--push] name newurl [oldurl]),
+   N_(git remote set-url [--fetch] [--push] --add name newurl),
+   N_(git remote set-url [--fetch] [--push] --delete name url),
NULL
 };
 
@@ -66,9 +66,9 @@ static const char * const builtin_remote_update_usage[] = {
 };
 
 static const char * const builtin_remote_seturl_usage[] = {
-   N_(git remote set-url [--push] name newurl [oldurl]),
-   N_(git remote set-url --add name newurl),
-   N_(git remote set-url --delete name url),
+   N_(git remote set-url [--fetch] [--push] name newurl [oldurl]),
+   N_(git remote set-url [--fetch] [--push] --add name newurl),
+   N_(git remote set-url [--fetch] [--push] --delete name url),
NULL
 };
 
@@ -1505,18 +1505,19 @@ static int set_branches(int argc, const char **argv)
 
 static int set_url(int argc, const char **argv)
 {
-   int i, push_mode = 0, add_mode = 0, delete_mode = 0;
+   int i, fetch_only = 0, push_only = 0

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-19 Thread Peter Wu

On Wednesday 19 November 2014 14:08:00 Jeff King wrote:
 On Wed, Nov 19, 2014 at 04:18:02PM +0100, Peter Wu wrote:
 
  git remote set-url knew about the '--push' option to update just the
  pushurl, but it does not have a similar option for update fetch URL and
  leave whatever was in place for the push URL.
 
 Isn't that what:
 
   git remote set-url foo new-fetch-url
 
 does already? It affects only the url setting, which is the de-facto
 fetch setting (it is _also_ the push setting if there is no pushurl
 defined).

If pushurl is not set, then this will implicitly change the fetch URL
which is unwanted and fixed in this patch by introducing a --fetch
option.

 You gave this example:
 
  git remote add gh g...@github.com:Lekensteyn/git.git
  # Avoid needing SSH for pulling from a repo, so change fetch URL
  git remote set-url https://github.com/Lekensteyn/git.git
  # Hmm, the fetch URL got changed too, let's fix that.
  git remote add --push gh g...@github.com:Lekensteyn/git.git
 
 But here you do not have a pushurl defined in the first place. So I
 guess this is really just a shortcut for swapping the two, like:
 
   git remote set-url --push gh $(git config remote.gh.url)
   git remote set-url gh new-fetch-url

Indeed, and not having a push URL is not uncommon (that is, always the
case when a new remote is added or just cloned). Compare the above
against this one:

git remote set-url --fetch new-fetch-url

This is less verbose and is much more intuitive.

 I dunno. I guess that is more convenient, but it seems like a lot of
 code for a very marginal use case. But more importantly, I'm a little
 worried that the presence of --fetch creates confusion about what
 set-url without a --fetch or --push does. That is, it implies to me
 that:
 
   git remote add gh old-url
   git remote set-url gh --push push-url
   git remote set-url gh new-url
 
 would replace both the url _and_ pushurl values in the third step,
 since we did not specify --fetch.  But it is in fact identical whether
 you run it with --fetch or not.  That is, it creates a weirdly
 non-orthogonal interface.

Step three currently only replaces the fetch URL as an explicit push URL
(remote.gh.pushurl) is set in step two (and therefore remote.gh.url does
not become the implicit push URL).

This might be a bug, but since it has been so long this way I was
worried that people actually rely on this behavior. The following sets
both url and pushurl (even if the URLs are equal):

git remote set-url --push --fetch gh pushurl

The patch is not tiny, but also not overly large either even if it has
similar pieces of code duplicated. Care has been taken of to keep
backwards compatibility which has also increased the size.

By the way, in the old code there was a memleak as strbuf was not
released at the end of the function, isn't it? A new patch can be found
on the bottom, it fixes an issue in the tests (no change in other
parts).
-- 
Kind regards,
Peter
https://lekensteyn.nl
From 6be164f68b4f625ac90f998ae4073e031ebed8ba Mon Sep 17 00:00:00 2001
From: Peter Wu pe...@lekensteyn.nl
Date: Wed, 19 Nov 2014 15:57:40 +0100
Subject: [PATCHv2] remote: add new --fetch option for set-url

git remote set-url knew about the '--push' option to update just the
pushurl, but it does not have a similar option for update fetch URL and
leave whatever was in place for the push URL.

This patch adds support for a '--fetch' option which implements that use
case in a backwards compatible way: if neither --push nor --fetch are
given, then the push URL is modified too if it was not set before. This
is the case since the push URL is implicitly based on the fetch URL.

The documentation has also been updated and a missing '--push' option
is added to the 'git remote -h' command.

Tests are also added to verify the documented behavior.

Signed-off-by: Peter Wu pe...@lekensteyn.nl
---
 Documentation/git-remote.txt |  12 +++--
 builtin/remote.c | 106 +--
 t/t5505-remote.sh|  54 ++
 3 files changed, 134 insertions(+), 38 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index cb103c8..7bb21a2 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,9 +15,9 @@ SYNOPSIS
 'git remote remove' name
 'git remote set-head' name (-a | --auto | -d | --delete | branch)
 'git remote set-branches' [--add] name branch...
-'git remote set-url' [--push] name newurl [oldurl]
-'git remote set-url --add' [--push] name newurl
-'git remote set-url --delete' [--push] name url
+'git remote set-url' [--fetch] [--push] name newurl [oldurl]
+'git remote set-url' [--fetch] [--push] '--add' name newurl
+'git remote set-url' [--fetch] [--push] '--delete' name url
 'git remote' [-v | --verbose] 'show' [-n] name...
 'git remote prune' [-n | --dry-run] name...
 'git remote' [-v | --verbose] 'update' [-p | --prune] [(group | remote)...]
@@ -134,7

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-19 Thread Peter Wu

On Wednesday 19 November 2014 15:17:21 Jeff King wrote:
 On Wed, Nov 19, 2014 at 08:42:19PM +0100, Peter Wu wrote:
  git remote set-url --fetch new-fetch-url
  
  This is less verbose and is much more intuitive.
 
 I agree your suggestion is a nicer way to do this. I'm just not sure
 that this swapping is all that common an operation. If there were no
 cost, I wouldn't mind. But I'm mostly concerned about the funny,
 non-intuitive behavior of git remote set-url foo that is created by
 this.

It is not about swapping, but setting a new fetch while keeping the push
URL intact. Likely not common, but nice to have for the times it is
needed. I am sure there are other features which are not used a lot, but
still exist ;)

   would replace both the url _and_ pushurl values in the third step,
   since we did not specify --fetch.  But it is in fact identical whether
   you run it with --fetch or not.  That is, it creates a weirdly
   non-orthogonal interface.
  
  Step three currently only replaces the fetch URL as an explicit push URL
  (remote.gh.pushurl) is set in step two (and therefore remote.gh.url does
  not become the implicit push URL).
  
  This might be a bug, but since it has been so long this way I was
  worried that people actually rely on this behavior.
 
 I don't think this is a bug. I think that git fetch set-url without
 --push is a de-facto --fetch already. Which makes sense, as there
 isn't a --fetch now (and the --push variant and pushurl grew after
 the fact, so the url option serves double-duty as both the single url
 and the fetch half).
 
 And that's what makes the proposed interface funny. Omitting --fetch
 is already a de-facto --fetch, and sometimes the two behave the same,
 and sometimes differently. Calling the option --keep-push would be a
 more accurate description, but that is rather clunky.
 
Before this patch I did not even know that git remote set-url name url
would have different user-visible behavior depending on whether a
pushurl is set or not. In my opinion, the proposed functionality does
not make the interface more confusing. Instead, the new option establish
a behavior which is consistent with the existing '--push' name.

(Aside, I intended to name this option --pull which seemed more
natural given the opposite direction --push, but decided to stay
consistent with the git remote show terminology.)

I think that your confusion is caused by the meaning of '--push' and
'--fetch'. These options form a group and are not as independent as
--add. Something like --change=[push|fetch|all] would describe the
functionality better:

git remote set-url --change=fetch gh fetchurl

But then the --push option becomes redundant.
-- 
Kind regards,
Peter
https://lekensteyn.nl

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-19 Thread Peter Wu

On Wednesday 19 November 2014 12:29:47 Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
  I dunno. I guess that is more convenient, but it seems like a lot of
  code for a very marginal use case. But more importantly, I'm a little
  worried that the presence of --fetch creates confusion about what
  set-url without a --fetch or --push does. That is, it implies to me
  that:
 
git remote add gh old-url
git remote set-url gh --push push-url
git remote set-url gh new-url
 
  would replace both the url _and_ pushurl values in the third step,
  since we did not specify --fetch.  But it is in fact identical whether
  you run it with --fetch or not.  That is, it creates a weirdly
  non-orthogonal interface.
 
 Yes, the semantics the updated code gives feel very strange.  I
 wouldn't be able to write a three-line summary in the release notes
 to advertise what good this new feature brings to users myself.

What about:

git remote set-url learned a new --fetch option which can be
used to change the fetch URL while leaving the push URL intact.
Useful to keep a ssh URL for push and change the fetch URL to https.

which is effectively the functionality I am using it for.
-- 
Kind regards,
Peter
https://lekensteyn.nl

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

2014-11-19 Thread Peter Wu

On Wednesday 19 November 2014 13:18:56 Junio C Hamano wrote:
 Junio C Hamano gits...@pobox.com writes:
  Jeff King p...@peff.net writes:
  If you are fetching from somebody else and then pushing into your
  own publishing repository (i.e. fork of that upstream), why isn't
  the sequence of event like this, instead?
 
  $ git clone $upstream
  $ browser github.com
  ... fork upstream to your own publishing repository ...
  $ git remote set-url --push mine url for your publish repo
 
  Isn't this one of those bad workflows encouraged by GitHub, for
  which you guys have to be punished ;-)?

For forks, it usually goes like this:

git clone $upstream
... realizes that is has a bug which I want to fix ...
... creates a new repo ...
git remote rename origin upstream
git remote add origin git@$personal_repo
# --fetch is what I need
git remote add --fetch https://$personal_repo

I often start by entering/copying the ssh URL which is what I need for
pushing. Later ssh-agent forget about my key and I realize that push
works fine over https, so would like to set that... only to observe that
is not possible in an straightforward way through 'git remote'.

 Coming back to the topic, how common would this oops, I cloned via
 a wrong transport be?  I am not opposed to giving a recovery method
 for gotcha that does not happen very often, but if such an addition
 adds undue confusion factor for people who use set-url for more
 common cases, that would be a bad trade-off.

Well, people rarely need to use 'git remote' except when, well, they
need to modify the remotes. Where does the confusion come from? I might
be biased now that I know the internals. Maybe the https/ssh case above
needs to be mentioned in the documentation? What do you think of the
updated documentation by the way?
-- 
Kind regards,
Peter
https://lekensteyn.nl

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git archive and glob pathspecs

2014-09-13 Thread Peter Wu

On Wednesday 03 September 2014 13:21:06 Duy Nguyen wrote:
 On Wed, Sep 3, 2014 at 5:17 AM, Peter Wu pe...@lekensteyn.nl wrote:
  Hi,
 
  The `git archive` seems to accept a pathspec judging from the error message 
  (git
  version 2.1.0):
 
  git archive HEAD -- :x
  fatal: pathspec 'x' did not match any files
 
  When I try to use deeper glob specs however, it throws an error (this also
  happens if I use `:(glob)**/Makefile`, tested in the git source tree):
 
  $ git archive HEAD -- ':(glob)*/Makefile'
  fatal: pathspec '*/Makefile' did not match any files
 
  Strange enough, command `git log -- ':(glob)*/Makefile'` works. Any idea 
  what is
  wrong?
 
 There may be something wrong. This patch seems to make it work for me,
 but it includes lots of empty directories. I'll have a closer look
 later (btw it's surprising that negative pathspec works too..)

I can confirm that this patch shows Makefile's, but also includes a lot of empty
directories.

As for why this happens, my guess is that write_archive_entries() recurses the
full tree and adds every encountered directory (via read_tree_1, via
write_archive_entry()).

To fix this, write_archive (write_tar_archive, etc.) should be taught to handle
glob patterns, or parse_pathspec should expand globs (and then
parse_pathspec_arg might have to validate the remaining patterns).

Kind regards,
Peter

 diff --git a/archive.c b/archive.c
 index 3fc0fb2..a5be58d 100644
 --- a/archive.c
 +++ b/archive.c
 @@ -221,6 +221,7 @@ static int path_exists(struct tree *tree, const char 
 *path)
   int ret;
 
   parse_pathspec(pathspec, 0, 0, , paths);
 + pathspec.recursive = 1;
   ret = read_tree_recursive(tree, , 0, 0, pathspec, reject_entry, NULL);
   free_pathspec(pathspec);
   return ret != 0;
 @@ -237,6 +238,7 @@ static void parse_pathspec_arg(const char **pathspec,
   parse_pathspec(ar_args-pathspec, 0,
 PATHSPEC_PREFER_FULL,
 , pathspec);
 + ar_args-pathspec.recursive = 1;
   if (pathspec) {
   while (*pathspec) {
   if (**pathspec  !path_exists(ar_args-tree, *pathspec))

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] archive: support filtering paths with glob

2014-09-13 Thread Peter Wu

On Thursday 04 September 2014 20:37:52 Nguyễn Thái Ngọc Duy wrote:
 This patch fixes two problems with using :(glob) (or even *.c
 without :(glob)).
 
 The first one is we forgot to turn on the 'recursive' flag in struct
 pathspec. Without that, tree_entry_interesting() will not mark
 potential directories interesting so that it can confirm whether
 those directories have anything matching the pathspec.
 
 The marking directories interesting has a side effect that we need to
 walk inside a directory to realize that there's nothing interested in
 there. By that time, 'archive' code has already written the (empty)
 directory down. That means lots of empty directories in the result
 archive.
 
 This problem is fixed by lazily writing directories down when we know
 they are actually needed. There is a theoretical bug in this
 implementation: we can't write empty trees/directories that match that
 pathspec.
 
 Noticed-by: Peter Wu pe...@lekensteyn.nl
 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com

Ah, ignore my last mail, I just noticed this one. This patch works fine. By the
way, the glob pattern is treated as a 'nullglob'. If you specify a glob pattern
that matches nothing, an empty archive is created. Perhaps an error message
should be raised for that as it is unlikely that a user wants that?

Tested-by: Peter Wu pe...@lekensteyn.nl

 ---
  Similar approach could probably be used for teaching ls-tree about pathspec..
 
  archive.c   | 82 
 -
  t/t5000-tar-tree.sh | 10 +++
  2 files changed, 91 insertions(+), 1 deletion(-)
 
 diff --git a/archive.c b/archive.c
 index 3fc0fb2..9e62bf4 100644
 --- a/archive.c
 +++ b/archive.c
 @@ -98,9 +98,19 @@ static void setup_archive_check(struct git_attr_check 
 *check)
   check[1].attr = attr_export_subst;
  }
  
 +struct directory {
 + struct directory *up;
 + unsigned char sha1[20];
 + int baselen, len;
 + unsigned mode;
 + int stage;
 + char path[FLEX_ARRAY];
 +};
 +
  struct archiver_context {
   struct archiver_args *args;
   write_archive_entry_fn_t write_entry;
 + struct directory *bottom;
  };
  
  static int write_archive_entry(const unsigned char *sha1, const char *base,
 @@ -146,6 +156,65 @@ static int write_archive_entry(const unsigned char 
 *sha1, const char *base,
   return write_entry(args, sha1, path.buf, path.len, mode);
  }
  
 +static void queue_directory(const unsigned char *sha1,
 + const char *base, int baselen, const char *filename,
 + unsigned mode, int stage, struct archiver_context *c)
 +{
 + struct directory *d;
 + d = xmallocz(sizeof(*d) + baselen + 1 + strlen(filename));
 + d-up  = c-bottom;
 + d-baselen = baselen;
 + d-mode= mode;
 + d-stage   = stage;
 + c-bottom  = d;
 + d-len = sprintf(d-path, %.*s%s/, baselen, base, filename);
 + hashcpy(d-sha1, sha1);
 +}
 +
 +static int write_directory(struct archiver_context *c)
 +{
 + struct directory *d = c-bottom;
 + int ret;
 +
 + if (!d)
 + return 0;
 + c-bottom = d-up;
 + d-path[d-len - 1] = '\0'; /* no trailing slash */
 + ret =
 + write_directory(c) ||
 + write_archive_entry(d-sha1, d-path, d-baselen,
 + d-path + d-baselen, d-mode,
 + d-stage, c) != READ_TREE_RECURSIVE;
 + free(d);
 + return ret ? -1 : 0;
 +}
 +
 +static int queue_or_write_archive_entry(const unsigned char *sha1,
 + const char *base, int baselen, const char *filename,
 + unsigned mode, int stage, void *context)
 +{
 + struct archiver_context *c = context;
 +
 + while (c-bottom 
 +!(baselen = c-bottom-len 
 +  !strncmp(base, c-bottom-path, c-bottom-len))) {
 + struct directory *next = c-bottom-up;
 + free(c-bottom);
 + c-bottom = next;
 + }
 +
 + if (S_ISDIR(mode)) {
 + queue_directory(sha1, base, baselen, filename,
 + mode, stage, c);
 + return READ_TREE_RECURSIVE;
 + }
 +
 + if (write_directory(c))
 + return -1;
 + return write_archive_entry(sha1, base, baselen, filename, mode,
 +stage, context);
 +}
 +
  int write_archive_entries(struct archiver_args *args,
   write_archive_entry_fn_t write_entry)
  {
 @@ -167,6 +236,7 @@ int write_archive_entries(struct archiver_args *args,
   return err;
   }
  
 + memset(context, 0, sizeof(context));
   context.args = args;
   context.write_entry = write_entry;
  
 @@ -187,9 +257,17 @@ int write_archive_entries(struct archiver_args *args,
   }
  
   err = read_tree_recursive(args-tree, , 0, 0, args-pathspec,
 -   write_archive_entry, context);
 +   args

Re: [PATCH 2/2] branch: let branch filters imply --list

2013-01-31 Thread Peter Wu

On Thursday 31 January 2013 01:46:11 Jeff King wrote:
 Currently, a branch filter like `--contains`, `--merged`, or
 `--no-merged` is ignored when we are not in listing mode.
 For example:
 
   git branch --contains=foo bar
 
 will create the branch bar from the current HEAD, ignoring
 the `--contains` argument entirely. This is not very
 helpful. There are two reasonable behaviors for git here:
 
   1. Flag an error; the arguments do not make sense.
 
   2. Implicitly go into `--list` mode
 
 This patch chooses the latter, as it is more convenient, and
 there should not be any ambiguity with attempting to create
 a branch; using `--contains` and not wanting to list is
 nonsensical.
 
 That leaves the case where an explicit modification option
 like `-d` is given.  We already catch the case where
 `--list` is given alongside `-d` and flag an error. With
 this patch, we will also catch the use of `--contains` and
 other filter options alongside `-d`.
 
 Signed-off-by: Jeff King p...@peff.net
Tested-by: Peter Wu lekenst...@gmail.com

I have tested this patch on top of 1.8.1.2 and it seems to work.

One note, the following command spits out master without complaining about the 
non-existing branch name:

git branch --contains id master non-existant branch name

(the order of branches doesn't affect the result.)

Regards,
Peter
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git-svn: do not escape certain characters in paths

2013-01-17 Thread Peter Wu

Subversion 1.7 and newer implement HTTPv2, an extension that should make HTTP
more efficient. Servers with support for this protocol will make the subversion
client library take an alternative code path that checks (with assertions)
whether the URL is canonical or not.

This patch fixes an issue I encountered while trying to `git svn dcommit` a
rename action for a file containing a single quote character (User's Manual
to UserMan.tex). It does not happen for older subversion 1.6 servers nor
non-HTTP(S) protocols such as the native svn protocol, only on an Apache server
shipping SVN 1.7. Trying to `git svn dcommit` under the aforementioned
conditions yields the following error which aborts the commit process:

Committing to http://example.com/svn ...
perl: subversion/libsvn_subr/dirent_uri.c:1520: uri_skip_ancestor:
Assertion `svn_uri_is_canonical(child_uri, ((void *)0))' failed.
error: git-svn died of signal 6

An analysis of the subversion source for the cause:

- The assertion originates from uri_skip_ancestor which calls
  svn_uri_is_canonical, which fails when the URL contains percent-encoded values
  that do not necessarily have to be encoded (not canonical enough). This is
  done by a table lookup in libsvn_subr/path.c. Putting some debugging prints
  revealed that the character ' is indeed encoded to %27 which is not
  considered canonical.
- url_skip_ancestor is called by svn_ra_neon__get_baseline_info with the root
  repository URL and path as parameters;
- which is called by copy_resource (libsvn_ra_neon/commit.c) for a copy action
  (or in my case, renaming which is actually copy + delete old);
- which is called by commit_add_dir;
- which is assigned as a structure method add_file in
  svn_ra_neon__get_commit_editor.

In the whole path, the path argument is not modified.

Through some more uninteresting wrapper functions, the Perl bindings gives you
access to the add_file method which will pass the path argument without
modifications to svn.

git-svn calls the R(ename) subroutine in Git::SVN::Editor which contains:
326 my $fbat = $self-add_file($self-repo_path($m-{file_b}), $pbat,
327 $self-url_path($m-{file_a}), $self-{r});
repo_path basically returns the path as-is, unless the svn.pathnameencoding
configuration property is set. url_path tries to escape some special
characters, but does not take all special characters into account, thereby
causing the path to contain some escaped characters which do not have to be
escaped.

The list of characters not to be escaped are taken from the
subversion/libsvn_subr/path.c file to fully account for all characters. Tested
with a filename containing all characters in the range 0x20 to 0x78 (inclusive).

Signed-off-by: Peter Wu lekenst...@gmail.com
---
 perl/Git/SVN/Editor.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm
index 3bbc20a..30f92cb 100644
--- a/perl/Git/SVN/Editor.pm
+++ b/perl/Git/SVN/Editor.pm
@@ -145,7 +145,8 @@ sub repo_path {
 sub url_path {
my ($self, $path) = @_;
if ($self-{url} =~ m#^https?://#) {
-   $path =~ s!([^~a-zA-Z0-9_./-])!uc sprintf(%%%02x,ord($1))!eg;
+   # characters are taken from subversion/libsvn_subr/path.c
+   $path =~ s#([^~a-zA-Z0-9_./!$'()*+,-])#uc 
sprintf(%%%02x,ord($1))#eg;
}
$self-{url} . '/' . $self-repo_path($path);
 }
-- 
1.8.1.1


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] bash-completion fixes for global git options handling

[PATCH 3/3] completion: handle git -C option

[PATCH 2/3] completion: pass --git-dir to more commands

[PATCH 1/3] completion: ignore git options for subcommand completion

Re: Git compile warnings (under mac/clang)

[PATCH v4] remote: add --fetch and --both options to set-url

Re: [PATCH v3] remote: add --fetch and --both options to set-url

Re: [PATCH v4] remote: add --fetch and --both options to set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[PATCH v3] remote: add --fetch and --both options to set-url

Re: [PATCH v3] remote: add --fetch and --both options to set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

[RFC] [PATCH] remote: add new --fetch option for set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Re: [RFC] [PATCH] remote: add new --fetch option for set-url

Re: git archive and glob pathspecs

Re: [PATCH] archive: support filtering paths with glob

Re: [PATCH 2/2] branch: let branch filters imply --list

[PATCH] git-svn: do not escape certain characters in paths

26 matches

Site Navigation

Mail list logo

Footer information