* [PATCH 4/4] push: add 'prune' option
From: Felipe Contreras @ 2012-02-22 22:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Felipe Contreras
In-Reply-To: <1329950621-21165-1-git-send-email-felipe.contreras@gmail.com>
This will allow us to remove refs from the remote that have been removed
locally.
It's useful to conveniently synchronize all the local branches to
certain remote.
Unfortunately, if we want to handle src:dst refspecs properly, some
extra complexity is needed, so check_pattern_match() needs to be
reorganized (previous patch).
This ensures that the following works:
$ git push --prune remote refs/heads/*:refs/tmp/*
So that local branches are not only pushed as custom refs in the remote,
but also refs/tmp/foo would be removed if there's no corresponding local
refs/heads/foo.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
Documentation/git-push.txt | 9 ++++++++-
builtin/push.c | 2 ++
remote.c | 31 ++++++++++++++++++++++++++++---
remote.h | 3 ++-
t/t5516-fetch-push.sh | 16 ++++++++++++++++
transport.c | 2 ++
transport.h | 1 +
7 files changed, 59 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index aede488..204f36d 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git push' [--all | --mirror | --tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
- [--repo=<repository>] [-f | --force] [-v | --verbose] [-u | --set-upstream]
+ [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]
[<repository> [<refspec>...]]
DESCRIPTION
@@ -71,6 +71,13 @@ nor in any Push line of the corresponding remotes file---see below).
Instead of naming each ref to push, specifies that all
refs under `refs/heads/` be pushed.
+--prune::
+ Remove remote branches that don't have a local counterpart. For example
+ a remote branch `tmp` will be removed if a local branch with the same
+ name doesn't exist any more. This also respects refspecs, e.g.
+ `refs/heads/*:refs/tmp/*` would make sure that remote `refs/tmp/foo`
+ will be removed if `refs/heads/foo` doesn't exist.
+
--mirror::
Instead of naming each ref to push, specifies that all
refs under `refs/` (which includes but is not
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..fdfb2c4 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
TRANSPORT_PUSH_SET_UPSTREAM),
OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+ OPT_BIT(0, "prune", &flags, "prune locally removed refs",
+ TRANSPORT_PUSH_PRUNE),
OPT_END()
};
diff --git a/remote.c b/remote.c
index 4709e20..9c590ae 100644
--- a/remote.c
+++ b/remote.c
@@ -8,6 +8,8 @@
#include "tag.h"
#include "string-list.h"
+enum map_direction { FROM_SRC, FROM_DST };
+
static struct refspec s_tag_refspec = {
0,
1,
@@ -1115,7 +1117,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
}
static char *get_ref_match(const struct refspec *rs, int rs_nr, const struct ref *ref,
- int send_mirror, const struct refspec **ret_pat)
+ int send_mirror, int direction, const struct refspec **ret_pat)
{
const struct refspec *pat;
char *name;
@@ -1130,7 +1132,12 @@ static char *get_ref_match(const struct refspec *rs, int rs_nr, const struct ref
if (rs[i].pattern) {
const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
- if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
+ int match;
+ if (direction == FROM_SRC)
+ match = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name);
+ else
+ match = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name);
+ if (match) {
matching_refs = i;
break;
}
@@ -1177,6 +1184,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
struct refspec *rs;
int send_all = flags & MATCH_REFS_ALL;
int send_mirror = flags & MATCH_REFS_MIRROR;
+ int send_prune = flags & MATCH_REFS_PRUNE;
int errs;
static const char *default_refspec[] = { ":", NULL };
struct ref *ref, **dst_tail = tail_ref(dst);
@@ -1197,7 +1205,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
if (ref->peer_ref)
continue;
- dst_name = get_ref_match(rs, nr_refspec, ref, send_mirror, &pat);
+ dst_name = get_ref_match(rs, nr_refspec, ref, send_mirror, FROM_SRC, &pat);
if (!dst_name)
continue;
@@ -1224,6 +1232,23 @@ int match_push_refs(struct ref *src, struct ref **dst,
free_name:
free(dst_name);
}
+ if (send_prune) {
+ /* check for missing refs on the remote */
+ for (ref = *dst; ref; ref = ref->next) {
+ char *src_name;
+
+ if (ref->peer_ref)
+ /* We're already sending something to this ref. */
+ continue;
+
+ src_name = get_ref_match(rs, nr_refspec, ref, send_mirror, FROM_DST, NULL);
+ if (src_name) {
+ if (!find_ref_by_name(src, src_name))
+ ref->peer_ref = alloc_delete_ref();
+ free(src_name);
+ }
+ }
+ }
if (errs)
return -1;
return 0;
diff --git a/remote.h b/remote.h
index b395598..341142c 100644
--- a/remote.h
+++ b/remote.h
@@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);
enum match_refs_flags {
MATCH_REFS_NONE = 0,
MATCH_REFS_ALL = (1 << 0),
- MATCH_REFS_MIRROR = (1 << 1)
+ MATCH_REFS_MIRROR = (1 << 1),
+ MATCH_REFS_PRUNE = (1 << 2),
};
/* Reporting of tracking info */
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b69cf57..b5417cc 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -979,4 +979,20 @@ test_expect_success 'push --porcelain --dry-run rejected' '
test_cmp .git/foo .git/bar
'
+test_expect_success 'push --prune' '
+ mk_test heads/master heads/second heads/foo heads/bar &&
+ git push --prune testrepo &&
+ check_push_result $the_commit heads/master &&
+ check_push_result $the_first_commit heads/second &&
+ ! check_push_result $the_first_commit heads/foo heads/bar
+'
+
+test_expect_success 'push --prune refspec' '
+ mk_test tmp/master tmp/second tmp/foo tmp/bar &&
+ git push --prune testrepo "refs/heads/*:refs/tmp/*" &&
+ check_push_result $the_commit tmp/master &&
+ check_push_result $the_first_commit tmp/second &&
+ ! check_push_result $the_first_commit tmp/foo tmp/bar
+'
+
test_done
diff --git a/transport.c b/transport.c
index cac0c06..c20267c 100644
--- a/transport.c
+++ b/transport.c
@@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport,
match_flags |= MATCH_REFS_ALL;
if (flags & TRANSPORT_PUSH_MIRROR)
match_flags |= MATCH_REFS_MIRROR;
+ if (flags & TRANSPORT_PUSH_PRUNE)
+ match_flags |= MATCH_REFS_PRUNE;
if (match_push_refs(local_refs, &remote_refs,
refspec_nr, refspec, match_flags)) {
diff --git a/transport.h b/transport.h
index 059b330..ce99ef8 100644
--- a/transport.h
+++ b/transport.h
@@ -102,6 +102,7 @@ struct transport {
#define TRANSPORT_PUSH_PORCELAIN 16
#define TRANSPORT_PUSH_SET_UPSTREAM 32
#define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
+#define TRANSPORT_PUSH_PRUNE 128
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
--
1.7.9.1
^ permalink raw reply related
* [PATCH 3/4] remote: refactor code into alloc_delete_ref()
From: Felipe Contreras @ 2012-02-22 22:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Felipe Contreras
In-Reply-To: <1329950621-21165-1-git-send-email-felipe.contreras@gmail.com>
Will be useful in next patches. No functional changes.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
remote.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/remote.c b/remote.c
index 9d29acc..4709e20 100644
--- a/remote.c
+++ b/remote.c
@@ -978,16 +978,20 @@ static void tail_link_ref(struct ref *ref, struct ref ***tail)
*tail = &ref->next;
}
+static struct ref *alloc_delete_ref(void)
+{
+ struct ref *ref = alloc_ref("(delete)");
+ hashclr(ref->new_sha1);
+ return ref;
+}
+
static struct ref *try_explicit_object_name(const char *name)
{
unsigned char sha1[20];
struct ref *ref;
- if (!*name) {
- ref = alloc_ref("(delete)");
- hashclr(ref->new_sha1);
- return ref;
- }
+ if (!*name)
+ return alloc_delete_ref();
if (get_sha1(name, sha1))
return NULL;
ref = alloc_ref(name);
--
1.7.9.1
^ permalink raw reply related
* [PATCH 0/4] push: add 'prune' option
From: Felipe Contreras @ 2012-02-22 22:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Felipe Contreras
Hi,
As mentioned in a previous thread[1], git is lacking some functionality to
synchronize completely with remote repositories.
As an example I put my use-case; I want to backup *all* my local branches to a
personal repository, and I want to remove branches that I have removed from my
local repository. git push personal 'refs/heads/*' mostly does the job, but it
doesn't remove anything, and that's where 'prune' comes from.
Do not confuse the remote branches with the upstream ones; you could have two
repositories where you want to synchronize branches to:
% git push --prune --force backup1 'refs/heads/*'
% git push --prune --force backup2 'refs/heads/*'
I still think a 'git remote sync' would be tremendously useful, but it can
actually use this --prune option.
Cheers.
[1] http://article.gmane.org/gmane.comp.version-control.git/184990
Since RFC:
- Most of comments addressed
- Documentation and tests added
Felipe Contreras (4):
remote: use a local variable in match_push_refs()
remote: reorganize check_pattern_match()
remote: refactor code into alloc_delete_ref()
push: add 'prune' option
Documentation/git-push.txt | 9 +++-
builtin/push.c | 2 +
remote.c | 107 ++++++++++++++++++++++++++++----------------
remote.h | 3 +-
t/t5516-fetch-push.sh | 16 +++++++
transport.c | 2 +
transport.h | 1 +
7 files changed, 100 insertions(+), 40 deletions(-)
--
1.7.9.1
^ permalink raw reply
* [PATCH 2/4] remote: reorganize check_pattern_match()
From: Felipe Contreras @ 2012-02-22 22:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Felipe Contreras
In-Reply-To: <1329950621-21165-1-git-send-email-felipe.contreras@gmail.com>
There's a lot of code that can be consolidated there, and will be useful
for next patches.
Right now match_name_with_pattern() is called twice, the first one
without value and result, and the second one with them. Since
check_pattern_match() is only used in one place, we can just reorganize
it to make a single call and fetch the values at the same time.
Since this is a semantic change, also rename the function to
get_ref_match() which actually describes more closely what it's actually
doing now.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
remote.c | 59 ++++++++++++++++++++++++++++++-----------------------------
1 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/remote.c b/remote.c
index 55d68d1..9d29acc 100644
--- a/remote.c
+++ b/remote.c
@@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
return errs;
}
-static const struct refspec *check_pattern_match(const struct refspec *rs,
- int rs_nr,
- const struct ref *src)
+static char *get_ref_match(const struct refspec *rs, int rs_nr, const struct ref *ref,
+ int send_mirror, const struct refspec **ret_pat)
{
+ const struct refspec *pat;
+ char *name;
int i;
int matching_refs = -1;
for (i = 0; i < rs_nr; i++) {
@@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
continue;
}
- if (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,
- NULL, NULL))
- return rs + i;
+ if (rs[i].pattern) {
+ const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
+ if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
+ matching_refs = i;
+ break;
+ }
+ }
}
- if (matching_refs != -1)
- return rs + matching_refs;
- else
+ if (matching_refs == -1)
return NULL;
+
+ pat = rs + matching_refs;
+ if (pat->matching) {
+ /*
+ * "matching refs"; traditionally we pushed everything
+ * including refs outside refs/heads/ hierarchy, but
+ * that does not make much sense these days.
+ */
+ if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
+ return NULL;
+ name = xstrdup(ref->name);
+ }
+ if (ret_pat)
+ *ret_pat = pat;
+ return name;
}
static struct ref **tail_ref(struct ref **head)
@@ -1171,36 +1189,19 @@ int match_push_refs(struct ref *src, struct ref **dst,
struct ref *dst_peer;
const struct refspec *pat = NULL;
char *dst_name;
+
if (ref->peer_ref)
continue;
- pat = check_pattern_match(rs, nr_refspec, ref);
- if (!pat)
+ dst_name = get_ref_match(rs, nr_refspec, ref, send_mirror, &pat);
+ if (!dst_name)
continue;
- if (pat->matching) {
- /*
- * "matching refs"; traditionally we pushed everything
- * including refs outside refs/heads/ hierarchy, but
- * that does not make much sense these days.
- */
- if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
- continue;
- dst_name = xstrdup(ref->name);
-
-
- } else {
- const char *dst_side = pat->dst ? pat->dst : pat->src;
- if (!match_name_with_pattern(pat->src, ref->name,
- dst_side, &dst_name))
- die("Didn't think it matches any more");
- }
dst_peer = find_ref_by_name(*dst, dst_name);
if (dst_peer) {
if (dst_peer->peer_ref)
/* We're already sending something to this ref. */
goto free_name;
-
} else {
if (pat->matching && !(send_all || send_mirror))
/*
--
1.7.9.1
^ permalink raw reply related
* Re: [PATCHv4] git-p4: add initial support for RCS keywords
From: Luke Diamand @ 2012-02-22 22:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Scouten, Pete Wyckoff
In-Reply-To: <7vy5ruacpa.fsf@alter.siamese.dyndns.org>
On 22/02/12 19:29, Junio C Hamano wrote:
> Pete Wyckoff<pw@padd.com> writes:
>
>>> Improved-by: Pete Wyckoff<pw@padd.com>
>>> Signed-off-by: Luke Diamand<luke@diamand.org>
>>
>> Looks brilliant. Ack. Thanks for suffering through N rounds of
>> review. :)
>
> Well, I hate to say that I need to ask another round, to redo this patch
> on top of ld/git-p4-expanded-keywords topic that has already been in
> 'next'; a patch that replaces what is in 'next' will lose fix-ups for
> issues I pointed out in the first round that you forgot to follow and were
> fixed up locally by me when I queued the existing one.
>
> When working on an improvement to what you have sent out, please make it a
> habit of comparing your result with what are already queued, even when the
> earlier patches are still in 'pu'. They often are polished with trivial
> improvements (both to the patch and the log message) based on review
> comments from people when they are queued, which you do not want to lose.
Sorry - I had completely forgotten about that.
No need to apologize for asking me to rework this. I'm using this
amazing version control system that makes it really easy.
Now, if I was using Perforce, then by now I would pretty unhappy.... P
>
> Thanks.
^ permalink raw reply
* [PATCH] Documentation/merge-options.txt: group "ff" related options together
From: Junio C Hamano @ 2012-02-22 22:27 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Sean Gies
In-Reply-To: <7vr4xm8qin.fsf@alter.siamese.dyndns.org>
The --ff-only option was not described next to --ff and --no-ff options in
"git merge" documentation, even though these three are logically together,
describing one option that takes one of three values.
Also the description for '--ff' and '--no-ff' discussed what '--ff' means,
and mentioned '--no-ff' as if it were a side-note to '--ff'.
Make them into three top-level entries and list them together. This way,
it would be more clear that the user can choose one from these three.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Perhaps a patch like this would be an improvement to make it easier to
find the options.
Documentation/merge-options.txt | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index f2f1d0f..0bcbe0a 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -24,13 +24,18 @@ updated behaviour, the environment variable `GIT_MERGE_AUTOEDIT` can be
set to `no` at the beginning of them.
--ff::
+ When the merge resolves as a fast-forward, only update the branch
+ pointer, without creating a merge commit. This is the default
+ behavior.
+
--no-ff::
- Do not generate a merge commit if the merge resolved as
- a fast-forward, only update the branch pointer. This is
- the default behavior of git-merge.
-+
-With --no-ff Generate a merge commit even if the merge
-resolved as a fast-forward.
+ Create a merge commit even when the merge resolves as a
+ fast-forward.
+
+--ff-only::
+ Refuse to merge and exit with a non-zero status unless the
+ current `HEAD` is already up-to-date or the merge can be
+ resolved as a fast-forward.
--log[=<n>]::
--no-log::
@@ -65,11 +70,6 @@ merge.
With --no-squash perform the merge and commit the result. This
option can be used to override --squash.
---ff-only::
- Refuse to merge and exit with a non-zero status unless the
- current `HEAD` is already up-to-date or the merge can be
- resolved as a fast-forward.
-
-s <strategy>::
--strategy=<strategy>::
Use the given merge strategy; can be supplied more than
^ permalink raw reply related
* Re: Bug report: "git-merge --ff" should fail if branches have diverged
From: Junio C Hamano @ 2012-02-22 22:14 UTC (permalink / raw)
To: Sean Gies; +Cc: git@vger.kernel.org
In-Reply-To: <DB0837FD-0963-4BF9-BD7B-B243F580CC1C@imgtec.com>
Sean Gies <Sean.Gies@imgtec.com> writes:
> When I specify the "--ff" option to git-merge, I expect it to perform a
> fast-forward merge or none at all.
That expectation needs to be adjusted with s/--ff/--ff-only/, I think.
The "--no-ff" option says "Never do fast-forward and always create extra
commit even when the side branch is a descendant", and the "--ff" option
is a way to countermand it, i.e. "It is ok to fast-forward this merge".
And there is another one "I do not want anything bug fast forward", and
that is spelled --ff-only.
^ permalink raw reply
* gitk: set uicolor SystemButtonFace error on X11 if .gitk created using Win32 tk
From: Matt Seitz (matseitz) @ 2012-02-22 22:13 UTC (permalink / raw)
To: git
Would you please change gitk to not hard-code Win32-specific color
values when creating .gitk on a Win32 windowing system?
Gitk stopped working for me on Cygwin when Cygwin changed from using a
Win32 native version of tk to using the standard X11 version. The error
was because gitk had previously created a .gitk file using Win32
specific color values:
https://github.com/gitster/git/commit/1924d1bc0dc99cd3460d3551671908cc76
c09d3b
I was able to work around the problem by replacing the Win32 specific
colors in my .gitk file with the default colors gitk uses on other
windowing systems.
See also:
http://cygwin.com/ml/cygwin/2012-02/msg00391.html
^ permalink raw reply
* [PATCH v2] contrib: added git-diffall
From: Tim Henigan @ 2012-02-22 22:12 UTC (permalink / raw)
To: git, gitster; +Cc: davvid, stefano.lattarini, tim.henigan
This script adds directory diff support to git. It launches a single
instance of the user-configured external diff tool and performs a
directory diff between the specified revisions. The before/after files
are copied to a tmp directory to do this. Either 'diff.tool' or
'merge.tool' must be set before running the script.
The existing 'git difftool' command already allows the user to view diffs
using an external tool. However if multiple files have changed, a
separate instance of the tool is launched for each one. This can be
tedious when many files are involved.
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
This script has been hosted on GitHub [1] since April 2010. Enough people
have found it useful that I hope it will be considered for inclusion in
the standard git install, either in contrib or as a new core command.
Changes since v1:
- Changed to #!/bin/sh
- Eliminated use of 'which' statements
- Fixed trap function to actually run on abnormal exit
- Simplified path concatenation logic ($IFS)
- Corrected indentation errors
- Improved readability of while loop
- Cleaned up quoting of variables
This matches commit 5d4b90de3 on GitHub [1].
[1]: https://github.com/thenigan/git-diffall
contrib/diffall/README | 18 +++
contrib/diffall/git-diffall | 255 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 273 insertions(+)
create mode 100644 contrib/diffall/README
create mode 100755 contrib/diffall/git-diffall
diff --git a/contrib/diffall/README b/contrib/diffall/README
new file mode 100644
index 0000000..12881d2
--- /dev/null
+++ b/contrib/diffall/README
@@ -0,0 +1,18 @@
+The git-diffall script provides a directory based diff mechanism
+for git. The script relies on the diff.tool configuration option
+to determine what diff viewer is used.
+
+This script is compatible with all the forms used to specify a
+range of revisions to diff:
+
+ 1. git diffall: shows diff between working tree and staged changes
+ 2. git diffall --cached [<commit>]: shows diff between staged changes and HEAD (or other named commit)
+ 3. git diffall <commit>: shows diff between working tree and named commit
+ 4. git diffall <commit> <commit>: show diff between two named commits
+ 5. git diffall <commit>..<commit>: same as above
+ 6. git diffall <commit>...<commit>: show the changes on the branch containing and up to the second , starting at a common ancestor of both <commit>
+
+Note: all forms take an optional path limiter [--] [<path>*]
+
+This script is based on an example provided by Thomas Rast on the Git list [1]:
+[1] http://thread.gmane.org/gmane.comp.version-control.git/124807
diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
new file mode 100755
index 0000000..d942612
--- /dev/null
+++ b/contrib/diffall/git-diffall
@@ -0,0 +1,255 @@
+#!/bin/sh
+# Copyright 2010 - 2012, Tim Henigan <tim.henigan@gmail.com>
+#
+# Perform a directory diff between commits in the repository using
+# the external diff or merge tool specified in the user's config.
+
+USAGE='[--cached] [--copy-back] [-x|--extcmd=<command>] <commit>{0,2} -- <path>*
+
+ --cached Compare to the index rather than the working tree.
+
+ --copy-back Copy files back to the working tree when the diff
+ tool exits (in case they were modified by the
+ user). This option is only valid if the diff
+ compared with the working tree.
+
+ -x=<command>
+ --extcmd=<command> Specify a custom command for viewing diffs.
+ git-diffall ignores the configured defaults and
+ runs $command $LOCAL $REMOTE when this option is
+ specified. Additionally, $BASE is set in the
+ environment.
+'
+
+SUBDIRECTORY_OK=1
+. "$(git --exec-path)/git-sh-setup"
+
+TOOL_MODE=diff
+. "$(git --exec-path)/git-mergetool--lib"
+
+merge_tool="$(get_merge_tool)"
+if test -z "$merge_tool"
+then
+ echo "Error: Either the 'diff.tool' or 'merge.tool' option must be set."
+ usage
+fi
+
+start_dir=$(pwd)
+
+# needed to access tar utility
+cdup=$(git rev-parse --show-cdup) &&
+cd "$cdup" || {
+ echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
+ exit 1
+}
+
+# mktemp is not available on all platforms (missing from msysgit)
+# Use a hard-coded tmp dir if it is not available
+tmp="$(mktemp -d -t tmp.XXXXXX 2>/dev/null)" || {
+ tmp=/tmp/git-diffall-tmp
+}
+
+trap 'rm -rf "$tmp" 2>/dev/null' EXIT
+mkdir -p "$tmp"
+
+left=
+right=
+paths=
+path_sep=
+compare_staged=
+common_ancestor=
+left_dir=
+right_dir=
+diff_tool=
+copy_back=
+
+while test $# != 0
+do
+ case "$1" in
+ -h|--h|--he|--hel|--help)
+ usage
+ ;;
+ --cached)
+ compare_staged=1
+ ;;
+ --copy-back)
+ copy_back=1
+ ;;
+ -x|--e|--ex|--ext|--extc|--extcm|--extcmd)
+ diff_tool=$2
+ shift
+ ;;
+ --)
+ path_sep=1
+ ;;
+ -*)
+ echo Invalid option: "$1"
+ usage
+ ;;
+ *)
+ # could be commit, commit range or path limiter
+ case "$1" in
+ *...*)
+ left=${1%...*}
+ right=${1#*...}
+ common_ancestor=1
+ ;;
+ *..*)
+ left=${1%..*}
+ right=${1#*..}
+ ;;
+ *)
+ if test -n "$path_sep"
+ then
+ paths="$paths$1 "
+ elif test -z "$left"
+ then
+ left=$1
+ elif test -z "$right"
+ then
+ right=$1
+ else
+ paths="$paths$1 "
+ fi
+ ;;
+ esac
+ ;;
+ esac
+ shift
+done
+
+# Determine the set of files which changed
+if test -n "$left" && test -n "$right"
+then
+ left_dir="cmt-$(git rev-parse --short $left)"
+ right_dir="cmt-$(git rev-parse --short $right)"
+
+ if test -n "$compare_staged"
+ then
+ usage
+ elif test -n "$common_ancestor"
+ then
+ git diff --name-only "$left"..."$right" -- $paths > "$tmp/filelist"
+ else
+ git diff --name-only "$left" "$right" -- $paths > "$tmp/filelist"
+ fi
+elif test -n "$left"
+then
+ left_dir="cmt-$(git rev-parse --short $left)"
+
+ if test -n "$compare_staged"
+ then
+ right_dir="staged"
+ git diff --name-only --cached "$left" -- $paths > "$tmp/filelist"
+ else
+ right_dir="working_tree"
+ git diff --name-only "$left" -- $paths > "$tmp/filelist"
+ fi
+else
+ left_dir="HEAD"
+
+ if test -n "$compare_staged"
+ then
+ right_dir="staged"
+ git diff --name-only --cached -- $paths > "$tmp/filelist"
+ else
+ right_dir="working_tree"
+ git diff --name-only -- $paths > "$tmp/filelist"
+ fi
+fi
+
+# Exit immediately if there are no diffs
+if test ! -s "$tmp/filelist"
+then
+ exit 0
+fi
+
+if test -n "$copy_back" && test "$right_dir" != "working_tree"
+then
+ echo "--copy-back is only valid when diff includes the working tree."
+ exit 1
+fi
+
+# Create the named tmp directories that will hold the files to be compared
+mkdir -p "$tmp/$left_dir" "$tmp/$right_dir"
+
+# Populate the tmp/right_dir directory with the files to be compared
+if test -n "$right"
+then
+ while read name
+ do
+ ls_list=$(git ls-tree $right $name)
+ if test -n "$ls_list"
+ then
+ mkdir -p "$tmp/$right_dir/$(dirname "$name")"
+ git show "$right":"$name" > "$tmp/$right_dir/$name" || true
+ fi
+ done < "$tmp/filelist"
+elif test -n "$compare_staged"
+then
+ while read name
+ do
+ ls_list=$(git ls-files -- $name)
+ if test -n "$ls_list"
+ then
+ mkdir -p "$tmp/$right_dir/$(dirname "$name")"
+ git show :"$name" > "$tmp/$right_dir/$name"
+ fi
+ done < "$tmp/filelist"
+else
+ # Mac users have gnutar rather than tar
+ (tar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && tar -x)) || {
+ gnutar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && gnutar -x)
+ }
+fi
+
+# Populate the tmp/left_dir directory with the files to be compared
+while read name
+do
+ if test -n "$left"
+ then
+ ls_list=$(git ls-tree $left $name)
+ if test -n "$ls_list"
+ then
+ mkdir -p "$tmp/$left_dir/$(dirname "$name")"
+ git show "$left":"$name" > "$tmp/$left_dir/$name" || true
+ fi
+ else
+ if test -n "$compare_staged"
+ then
+ ls_list=$(git ls-tree HEAD $name)
+ if test -n "$ls_list"
+ then
+ mkdir -p "$tmp/$left_dir/$(dirname "$name")"
+ git show HEAD:"$name" > "$tmp/$left_dir/$name"
+ fi
+ else
+ mkdir -p "$tmp/$left_dir/$(dirname "$name")"
+ git show :"$name" > "$tmp/$left_dir/$name"
+ fi
+ fi
+done < "$tmp/filelist"
+
+cd "$tmp"
+LOCAL="$left_dir"
+REMOTE="$right_dir"
+
+if test -n "$diff_tool"
+then
+ export BASE
+ eval $diff_tool '"$LOCAL"' '"$REMOTE"'
+else
+ run_merge_tool "$merge_tool" false
+fi
+
+# Copy files back to the working dir, if requested
+if test -n "$copy_back" && test "$right_dir" = "working_tree"
+then
+ cd "$start_dir"
+ git_top_dir=$(git rev-parse --show-toplevel)
+ find "$tmp/$right_dir" -type f |
+ while read file
+ do
+ cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
+ done
+fi
--
1.7.9.GIT
^ permalink raw reply related
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Johannes Sixt @ 2012-02-22 22:10 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Rast, git, Jannis Pohlmann
In-Reply-To: <20120222205500.GD6781@sigill.intra.peff.net>
Am 22.02.2012 21:55, schrieb Jeff King:
> On Wed, Feb 22, 2012 at 08:34:23PM +0100, Thomas Rast wrote:
>> + printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - &&
>
> Seq is not portable.
Thanks for pointing this out.
> I usually use either
>
> perl -le "print for (1..100)"
>
> or just do:
>
> z16=zzzzzzzzzzzzzzzz
> z256=$z16$z16$z16$z16$z16$z16$z16$z16
> z1024=$z256$z256$z256$z256$z256$z256$z256$z256
If a sequence of ASCII zeros is good enough, you can also do:
printf %02134d 0
-- Hannes
^ permalink raw reply
* Re: [PATCHv2 0/8] gitweb: Faster and improved project search
From: Jakub Narebski @ 2012-02-22 22:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4numnfqq.fsf@alter.siamese.dyndns.org>
On Mon, 20 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > [Cc-ing Junio because of his involvement in discussion about first
> > patch in previous version of this series.]
> >
> > First three patches in this series are mainly about speeding up
> > project search (and perhaps in the future also project pagination).
> > Well, first one is unification, refactoring and future-proofing.
> > The second and third patch could be squashed together; second adds
> > @fill_only, but third actually uses it.
>
> I'll queue these three separately to a topic jn/gitweb-search-optim,
> and fork another topic jn/gitweb-hilite-regions from there. I haven't
> looked the latter closely, though.
O.K., when rerolling the series I will resend those in separate patch
series: one for performance improvements for project search (less calls
to git commands), one for match highlighting in project search ('grep',
'commit' and other per-project searches already highlight their matches,
though in suboptimal way), and perhaps one for using esc_html_match_hl()
thorough gitweb,... though with only one patch in this series for now
perhaps it be better joined with match highlighting for project search.
--
Jakub Narebski
Poland
^ permalink raw reply
* Bug report: "git-merge --ff" should fail if branches have diverged
From: Sean Gies @ 2012-02-22 21:51 UTC (permalink / raw)
To: git@vger.kernel.org
Git developers, I have a small bug report:
When I specify the "--ff" option to git-merge, I expect it to perform a fast-forward merge or none at all. If the branches have diverged and a fast-forward cannot be done, I expect the command to fail. With git 1.7.6, if "git-merge -ff" cannot fast-forward, it falls back to creating the merge commit I did not want.
Thank you,
-Sean
^ permalink raw reply
* Re: [PATCHv2 2/8] gitweb: Option for filling only specified info in fill_project_list_info
From: Jakub Narebski @ 2012-02-22 22:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfwe6ng8u.fsf@alter.siamese.dyndns.org>
On Mon, 20 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > Introduce project_info_needs_filling($pr, $key[, \%fill_only]), which
>
> That's "project_info_needs_filling($pr, @key[, \%fill_only])", isn't it?
Yes it is. I'm sorry, I have missed that this needs fixing from the
first version of patch series. Since then I have noticed that in few
cases we fill two fields at once, so we have to ask about two fields
at once as well.
> > +# entry for given @keys needs filling if at least one of interesting keys
> > +# in list is not present in %$project_info; key is interesting if $fill_only
> > +# is not passed, or is empty (all keys are interesting in both of those cases),
> > +# or if key is in $fill_only hash
> > +#
> > +# USAGE:
> > +# * project_info_needs_filling($project_info, 'key', ...)
> > +# * project_info_needs_filling($project_info, 'key', ..., \%fill_only)
> > +# where %fill_only = map { $_ => 1 } @fill_only;
>
> The reason this spells bad to me is that it gives the readers (and anybody
> who may want to touch gitweb code) that the caller has _two_ ways to say
> the same thing.
>
> If a caller is interested in finding out $key in $project_info, it can
> either (1) pass $key as one of the earlier parameters to the function and
> not pass any \%fill_only, or (2) pass $key and pass \%fill_only but after
> making sure \%fill_only also has $key defined.
>
> And if the caller passes \%fill_only, it has to remember that it needs to
> add $key to it; otherwise the earlier request "I want $key" is ignored by
> the function.
Yes, you are right. All of this is caused by fill_project_list_info()
trying to do *two* things, instead of doing one thing and doing it well,
as is Unix philosophy. It tried to check both that key is needed and
that key is wanted.
> Why is it a good thing to have such a convoluted API?
Thanks for a sanity check.
> Is it that "fill_project_list_info" is called by multiple callers, and
> they do not necessarily need to see all the fields that can be filled by
> the function (namely, age, age_string, descr, descr_long, owner, ctags and
> category)? If that is the case, I must say that the approach to put the
> set filtering logic inside project_info_needs_filling function smells like
> a bad API design.
You are right.
> In other words, wouldn't a callchain that uses a more natural set of API
> functions be like this?
>
> # the top-level caller that is interested only in these two fields
> fill_project_list_info($projlist, 'age', 'owner');
>
> # in fill_project_list_info()
> my $projlist = shift;
> my %wanted = map { $_ => 1 } @_;
>
> foreach my $pr (@$projlist) {
> if (project_info_needs_filling($pr,
> filter_set(\%wanted, 'age', 'age_string'))) {
That or
if (project_info_needs_filling($pr, 'age', 'age_string') &&
project_info_is_wanted(\%wanted, 'age', 'age_string')) {
and in your case there is no duplication of field names.
filter_set() is simply intersection of two sets, but it treats empty
%wanted as a full set.
> ... get @activity ...
> ($pr->{'age'}, $pr->{'age_string'}) = @activity;
> }
> ...
> }
>
> That way, "needs_filling" has to do one and only one thing well: it checks
> if any of the fields it was asked is missing and answers. And a generic
> set filtering helper that takes a filtering hashref and an array can be
> used later outside of the "needs_filling" function.
Right.
> > sub project_info_needs_filling {
> > + my $fill_only = ref($_[-1]) ? pop : undef;
> > my ($project_info, @keys) = @_;
> >
> > # return List::MoreUtils::any { !exists $project_info->{$_} } @keys;
> > foreach my $key (@keys) {
> > - if (!exists $project_info->{$key}) {
> > + if ((!$fill_only || !%$fill_only || $fill_only->{$key}) &&
> > + !exists $project_info->{$key}) {
> > return 1;
> > }
> > }
> > return;
> > }
> >
> > -
>
> Shouldn't the previous patch updated not to add this blank line in the
> first place?
O.K.
> > # fills project list info (age, description, owner, category, forks)
>
> Is this enumeration up-to-date? If we cannot keep it up-to-date, it may
> make sense to show only a few as examples and add ellipses.
O.K.
> > # for each project in the list, removing invalid projects from
> > -# returned list
> > +# returned list, or fill only specified info (removing invalid projects
> > +# only when filling 'age').
>
> It is unclear what the added clause wants to say; especially, the link
> between the mention of 'age' and 'only when' is unclear. Is asking of
> 'age' what makes a request a notable exception to remove invalid projects?
> Or is asking to partially fill fields what triggers the removal of invalid
> projects, and you mentioned 'age' as an example?
>
> If it is the former (I am guessing it is), it would be much clearer to
> say:
>
> Invalid projects are removed from the returned list if and only if you
> ask 'age' to be filled, because of such and such reasons.
O.K.
Sidenote: the reason is that to check that project is valid you really
have to run git command that requires repository in it[1]. Only when
filling 'age' and 'age_string' using git_get_last_activity() we always
run git command.
[1] Gitweb actually uses --git-dir=<PATH> parameter to git wrapper.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 2/4] completion: normalize increment/decrement style
From: Junio C Hamano @ 2012-02-22 22:05 UTC (permalink / raw)
To: Philip Jägenstedt
Cc: git, SZEDER Gábor, Felipe Contreras, Teemu Likonen
In-Reply-To: <1329901093-24106-3-git-send-email-philip@foolip.org>
Philip Jägenstedt <philip@foolip.org> writes:
> The style used for incrementing and decrementing variables was fairly
> inconsistenty and was normalized to use x++, or ((x++)) in contexts
> where the former would otherwise be interpreted as a command. This is a
> bash-ism, but for obvious reasons this script is already bash-specific.
>
> Signed-off-by: Philip Jägenstedt <philip@foolip.org>
I am not sure if the kind of changes seen in the following hunks are
necessary but I do not care too deeply about it. Assuming that the
change does not make the emulation by zsh unhappy, will apply.
Please stop me if somebody has issues with this rewrite.
Thanks.
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c63a408..1903bc9 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -178,10 +178,8 @@ __git_ps1_show_upstream ()
> for commit in $commits
> do
> case "$commit" in
> - "<"*) let ++behind
> - ;;
> - *) let ++ahead
> - ;;
> + "<"*) ((behind++)) ;;
> + *) ((ahead++)) ;;
> esac
> done
> count="$behind $ahead"
> @@ -739,7 +737,7 @@ __git_complete_remote_or_refspec ()
> local cur_="$cur" cmd="${words[1]}"
> local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
> if [ "$cmd" = "remote" ]; then
> - c=$((++c))
> + ((c++))
> fi
> while [ $c -lt $cword ]; do
> i="${words[c]}"
^ permalink raw reply
* Re: [PATCH] contrib: added git-diffall
From: Junio C Hamano @ 2012-02-22 21:06 UTC (permalink / raw)
To: Tim Henigan; +Cc: David Aguilar, git
In-Reply-To: <CAFouetgyYRKT7h1h4hv40Kxp=ibq1Hw-1mzFe6DsyAUknXKyYQ@mail.gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> I have not heard back from the user, but I tested on Ubuntu earlier
> today. I found that when using an older version of the script
> (fbedb7a in the GitHub repo), I could repeat the error. It is the
> 'git-diffall' script that fails silently, I believe due to a bash-ism
> that was fixed in a subsequent commit.
Thanks.
That would explain it, especially given that you've been doing:
> For what it is worth, since that bug was originally reported, I have
> been running "checkbashisms" [1] on the git-diffall script. That
> utility reports that the script is clean.
The checkbashisms scritp is interesting ;-)
I do not get much of the comment removal and string munging that happen
before it really start to check the contents for bash-isms, but most of
the check logic contained within the %foo_bashisms seem to be looking for
the right kind of mistakes people who are too used to bash make very
often.
^ permalink raw reply
* Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()
From: Junio C Hamano @ 2012-02-22 21:02 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <CAMP44s2HUG_ocHBaVpcsZHWMf2Tww+=bVun5H9+S5EGkoiJHRQ@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
>> For a change that not just adds parameters but removes an existing one,
>> this is way under-described with neither in-code comment nor log message.
>
> But it doesn't. src is renamed to ref.
That is exactly why I mentioned that this is a "chance" in the part of my
message you did not quote ;-).
It was a chance for you to learn how the thought process of others may
work or not work well, depending on how well the log message at the top
prepares their mind before they start to read the patch, by looking at how
one reader (me) tried to figure your patch out.
Anything that I said in the message you are responding to that tempts you
to say "No you are reading wrong" is an indication that the patch did not
do a good job to help the reader understand what you wanted to do.
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Jeff King @ 2012-02-22 21:00 UTC (permalink / raw)
To: Thomas Rast; +Cc: Junio C Hamano, Thomas Rast, git, Jannis Pohlmann
In-Reply-To: <87hayivcmm.fsf@thomas.inf.ethz.ch>
On Wed, Feb 22, 2012 at 09:25:53PM +0100, Thomas Rast wrote:
> > Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline
> > to begin with, except for debugging purposes.
> >
> > Do we ever use the subject string read from the rev-list output in any
> > way?
> >
> > In other words, I am wondering if the right patch to minimally fix the
> > issue starting from older releases is something along this line instead:
>
> Not sure. The only use I could think of would be to google for the
> subjects, in the hope of finding some repository that has the commit you
> are looking for. Other than that...
Or because the bundle creator (which may even be you on a different day)
did not correctly guess at the negative refs when specifying a cutoff.
Assuming you no longer have access to the original repo (not
unreasonable, since you are using a bundle), the messages could help you
understand where the error occurred.
Of course, once you figure out "aha! I expected the bundle to include
foo, but it is actually a cutoff point. I should have said XYZ^ as the
cutoff", I'm not sure what you do then. It's not like there is an easy
way to salvage the data that is in the bundle.
So I think it is a debugging aid, but one that ultimately doesn't help
you that much.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 3/3] push: add 'prune' option
From: Junio C Hamano @ 2012-02-22 20:56 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <CAMP44s3+XCM1E_AtW1yGifmGoGSkFSpSTaFbbMffz+hmUzWahw@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
>> Perhaps "enum map_direction { SRC_TO_DST, DST_TO_SRC }" or something?
>
> I think only FROM_SRC, FROM_DST is more than enough to figure it out.
Yeah, as you can tell from my "or something", as long as the name makes
the direction clear, I don't care too much about the exact spelling.
>> Perhaps rename this to "map_push_refs()" or something in the patch 2/3?
>
> I think get_ref_match() would be more appropriate because we are
> acting on a specific (singular) ref, and the primary thing we care
> about is getting the peer name, based on the refspec match, which we
> might want as a return value.
Again, as long as the name makes it clear this is meant only for "push"
and never be used for "fetch", I am fine with it. One way to make it sure
is to have a substring "_push" somewhere in that name.
> Probably some rebase mistake =/
Thanks; I was wondering if there is something subtle unexplained going on.
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Jeff King @ 2012-02-22 20:55 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jannis Pohlmann
In-Reply-To: <fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch>
On Wed, Feb 22, 2012 at 08:34:23PM +0100, Thomas Rast wrote:
> +# If "ridiculous" is at least 1004 chars, this traps a bug in old
> +# versions where the resulting 1025-char line (with --pretty=oneline)
> +# was longer than a 1024-char buffer
> +test_expect_success 'ridiculously long subject in boundary' '
> +
> + : > file4 &&
> + test_tick &&
> + git add file4 &&
> + printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - &&
Seq is not portable. I usually use either
perl -le "print for (1..100)"
or just do:
z16=zzzzzzzzzzzzzzzz
z256=$z16$z16$z16$z16$z16$z16$z16$z16
z1024=$z256$z256$z256$z256$z256$z256$z256$z256
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] bundle: put strbuf_readline_fd in strbuf.c with adjustments
From: Jeff King @ 2012-02-22 20:51 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jannis Pohlmann
In-Reply-To: <a795f6dca5e7c3fc5f9212becda4a46116c502b7.1329939233.git.trast@student.ethz.ch>
On Wed, Feb 22, 2012 at 08:34:22PM +0100, Thomas Rast wrote:
> The comment even said that it should eventually go there. While at
> it, match the calling convention and name of the function to the
> strbuf_get*line family. So it now is strbuf_getwholeline_fd.
> [...]
> bundle.c | 21 ++-------------------
> strbuf.c | 16 ++++++++++++++++
> strbuf.h | 1 +
Nit: no update to Documentation/technical/api-strbuf.txt.
You might want to also mention that this will read() one byte at a time,
and is therefore only a good idea if you really care about the position
of the fd. Otherwise, fdopen() + strbuf_getwholeline() is much more
efficient.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Junio C Hamano @ 2012-02-22 20:50 UTC (permalink / raw)
To: Thomas Rast; +Cc: Thomas Rast, git, Jannis Pohlmann
In-Reply-To: <87hayivcmm.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@inf.ethz.ch> writes:
> In any case the --pretty=oneline is very deliberate, as we can see from
> the commit below. It just doesn't give a reason :-)
Thanks for digging and I hope everybody on this list who would ever send a
patch learns the importance of describing _why_ in the log from this
lesson.
I'll amend your 2/2 with a note that keeping the subject material is not
justified and we may want to remove (or at least reduce) it in the future.
^ permalink raw reply
* Re: [RFC/PATCH 3/3] push: add 'prune' option
From: Felipe Contreras @ 2012-02-22 20:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vobsxqebz.fsf@alter.siamese.dyndns.org>
On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> This will allow us to remove refs from the remote that have been removed
>> locally.
>
> Can you enhance this a bit more to summarize the gist of what the semantic
> of this new feature is, perhaps like this:
>
> After pushing refs, "git push --prune" will remove refs from the
> remote that existed before the push and would have been pushed
> from us if we had some local refs that would have matched the
> refspecs used. For example,
>
> $ git push --prune remote refs/heads/*:refs/remotes/repo1/*
>
> will push all local branches in our repository to refs with
> corresponding names under refs/remotes/repo1/ at the remote, and
> removes remote's refs in refs/remotes/repo1/ that no longer have
> corresponding local branches in our repository. The refs at the
> remote outside refs/remotes/repo1/ are not affected.
>
> In order to alley the worries raised in the previous discussion, something
> to the effect of the last sentence above is crucial to have, I would think.
OK.
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>> OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
>> TRANSPORT_PUSH_SET_UPSTREAM),
>> OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
>> + OPT_BIT('p', "prune", &flags, "prune locally removed refs",
>> + TRANSPORT_PUSH_PRUNE),
>
> Please refrain from squatting on a short-and-sweet one letter option
> before this new feature proves to be popular and useful in a few cycles,
> especially when there already is a long option that begins with 'p'.
OK.
>> OPT_END()
>> };
>>
>> diff --git a/remote.c b/remote.c
>> index 019aafc..0900bb5 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1111,7 +1111,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
>> }
>>
>> static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
>> - int send_mirror, const struct refspec **ret_pat)
>> + int send_mirror, int dir, const struct refspec **ret_pat)
>
> Can we name this a bit better? I first thought "Huh? directory?", and had
> to scratch my head, wondering if it is an offset into refs/heads/* string
> or something....
OK.
>> {
>> const struct refspec *pat;
>> char *name;
>> @@ -1126,7 +1126,12 @@ static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref
>>
>> if (rs[i].pattern) {
>> const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
>> - if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
>> + int match;
>> + if (dir == 0)
>> + match = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name);
>> + else
>> + match = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name);
>
> ....until the code told us that it is some sort of direction of the
> matching. A symbolic constant or two would be even better.
>
> Originally this funcion was fed a list of refs in the source (i.e. on our
> end, as this is only used in 'push') and matched them against the source
> side of the refspec, rs[i].src, to see under what name the destination
> side will store it (i.e. give dst_side as value to find out the result in
> &name). This patch adds a new caller, who feeds a list of refs in the
> destination (i.e. on the remote end) to find out how they map to the names
> on our end (i.e. source). So "direction" is not necessarily incorrect; it
> is the direction this function maps the names (either src-to-dst for the
> original caller, or dst-to-src for the new caller).
>
> Perhaps "enum map_direction { SRC_TO_DST, DST_TO_SRC }" or something?
I think only FROM_SRC, FROM_DST is more than enough to figure it out.
>> + if (match) {
>> matching_refs = i;
>> break;
>> }
>
> So what is the updated semantics of this function? Is it still
> appropriate to name it "check_pattern_match()"?
>
> It seems that by now this does a lot more than just "check if a pattern
> matches". Since your patch 2/3, it is a function that finds out the
> refname in the remote that the given one refspec would try to update, and
> with this patch, it can also map in the reverse direction, given the list
> of remote refs, finding out which local ref a refspec would use to update
> them.
>
> At the same time, to reduce risk of future breakage, we probably should
> rename this function to make it clear that this function is to be only
> used by the push side.
>
> Perhaps rename this to "map_push_refs()" or something in the patch 2/3?
I think get_ref_match() would be more appropriate because we are
acting on a specific (singular) ref, and the primary thing we care
about is getting the peer name, based on the refspec match, which we
might want as a return value.
>> @@ -1173,6 +1178,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
>> struct refspec *rs;
>> int send_all = flags & MATCH_REFS_ALL;
>> int send_mirror = flags & MATCH_REFS_MIRROR;
>> + int send_prune = flags & MATCH_REFS_PRUNE;
>> int errs;
>> static const char *default_refspec[] = { ":", NULL };
>> struct ref *ref, **dst_tail = tail_ref(dst);
>> @@ -1193,7 +1199,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
>> if (ref->peer_ref)
>> continue;
>>
>> - dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);
>> + dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 0, &pat);
>> if (!dst_name)
>> continue;
>>
>> @@ -1220,6 +1226,23 @@ int match_push_refs(struct ref *src, struct ref **dst,
>> free_name:
>> free(dst_name);
>> }
>> + if (send_prune) {
>> + /* check for missing refs on the remote */
>> + for (ref = *dst; ref; ref = ref->next) {
>> + char *src_name;
>> +
>> + if (ref->peer_ref)
>> + /* We're already sending something to this ref. */
>> + continue;
>> +
>> + src_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 1, NULL);
>> + if (src_name) {
>> + if (!find_ref_by_name(src, src_name))
>> + ref->peer_ref = try_explicit_object_name("");
>
> Yuck. You do not want it to "try" as its name says. You just want to
> trigger its "delete" codepath.
>
> Please extract the body of "if (!*name) { ... }" block out of that
> function into a separate helper function, i.e.
>
> static struct ref *deleted_ref(void)
> {
> struct ref *ref = alloc_ref("(delete)");
> hashclr(ref->new_sha1);
> return ref;
> }
>
> then update try_explicit_...() to call it, and call the same helper here.
>
> This is not for runtime efficiency; feeding a constant to a function that
> says try_foo() or check_bar() that makes decision on the parameter only to
> trigger a partial codepath hurts readability.
All right.
>> + free(src_name);
>> + }
>> + }
>> + }
>> if (errs)
>> return -1;
>> return 0;
>> diff --git a/remote.h b/remote.h
>> index b395598..341142c 100644
>> --- a/remote.h
>> +++ b/remote.h
>> @@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);
>> enum match_refs_flags {
>> MATCH_REFS_NONE = 0,
>> MATCH_REFS_ALL = (1 << 0),
>> - MATCH_REFS_MIRROR = (1 << 1)
>> + MATCH_REFS_MIRROR = (1 << 1),
>> + MATCH_REFS_PRUNE = (1 << 2),
>> };
>>
>> /* Reporting of tracking info */
>> diff --git a/transport.c b/transport.c
>> index cac0c06..c20267c 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport,
>> match_flags |= MATCH_REFS_ALL;
>> if (flags & TRANSPORT_PUSH_MIRROR)
>> match_flags |= MATCH_REFS_MIRROR;
>> + if (flags & TRANSPORT_PUSH_PRUNE)
>> + match_flags |= MATCH_REFS_PRUNE;
>
> Does it make sense to specify --prune when --mirror is in effect? If so,
> how would it behave differently from a vanilla --mirror? If not, should
> it be detected as an error?
Probably doesn't make sense, should be an error.
> I couldn't infer from the context shown in the patch, but how in general
> does this new feature interact with the codepath for --mirror?
>
>> if (match_push_refs(local_refs, &remote_refs,
>> refspec_nr, refspec, match_flags)) {
>> diff --git a/transport.h b/transport.h
>> index 059b330..5d30328 100644
>> --- a/transport.h
>> +++ b/transport.h
>> @@ -101,6 +101,7 @@ struct transport {
>> #define TRANSPORT_PUSH_MIRROR 8
>> #define TRANSPORT_PUSH_PORCELAIN 16
>> #define TRANSPORT_PUSH_SET_UPSTREAM 32
>> +#define TRANSPORT_PUSH_PRUNE 64
>> #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
>
> Hrm...?
Probably some rebase mistake =/
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] remote-curl: Fix push status report when all branches fail
From: Jeff King @ 2012-02-22 20:40 UTC (permalink / raw)
To: Shawn Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <CAJo=hJsFDrt4rsxVAnx86bxZDY3yfWc1=GDd8opUU+9z7esLnw@mail.gmail.com>
On Wed, Feb 22, 2012 at 07:22:10AM -0800, Shawn O. Pearce wrote:
> > + /*
> > + * Ignore write errors; there's nothing we can do,
> > + * since we're about to close the pipe anyway. And the
> > + * most likely error is EPIPE due to the helper dying
> > + * to report an error itself.
> > + */
> > + sigchain_push(SIGPIPE, SIG_IGN);
> > + xwrite(data->helper->in, "\n", 1);
> > + sigchain_pop(SIGPIPE);
> [...]
>
> This sounds right to me. Its unfortunate that we missed the error
> status output when we built the remote helper protocol, but your patch
> above might be the best we can do now.
>
> Eh, well, actually we could have the helper advertise a new capability
> that can be enabled to return exit status. That is a much bigger
> change, and even if we do it for remote-curl (since that is in tree
> and easy to update) we still need your patch for the same race
> condition for out of tree helpers (which Google actually has so I care
> about out of tree helpers too).
I don't think it's worth a new capability. This is one of those "it
would be nice if it were designed that way from day one" cases, but it
wasn't. And while this is a minor hack, I don't think it has any
functional downsides. So adding a new capability on top of the hack just
makes things more complex.
I'll re-send the patch with a stand-alone commit message.
-Peff
^ permalink raw reply
* Re: Problems with unrecognized headers in git bundles
From: Øyvind A. Holm @ 2012-02-22 20:40 UTC (permalink / raw)
To: Jannis Pohlmann; +Cc: git
In-Reply-To: <CAA787rm4c1zYgQJ3kP5=ujpEK1Dda9+h_P3BBmg2yX2eZca=TA@mail.gmail.com>
On 22 February 2012 21:25, Øyvind A. Holm <sunny@sunbase.org> wrote:
> On 22 February 2012 17:05, Jannis Pohlmann wrote:
> > creating bundles from some repositories seems to lead to bundles
> > with incorrectly formatted headers, at least with git >= 1.7.2. When
> > cloning from such bundles, git prints the following error/warning:
> >
> > $ git clone perl-clone.bundle perl-clone
> > Cloning into 'perl-clone'...
> > warning: unrecognized header: --work around mangled archname on...
>
> Have researched this a bit, and I've found that all git versions back
> to when git-bundle was introduced (around v1.5.4) produces the same
> invalid line. The culprit is commit
> 3e8148feadabd0d0b1869fcc4d218a6475a5b0bc in perl.git, branch
> 'maint-5.005'. The log message of that commit contains email headers,
> maybe that's the reason git bundle gets confused?
...or maybe because the log message doesn't contain any empty lines, so
they're joined together into an insanely long line. I've seen this
behaviour before, in git-am or git-apply, I think. Anyway, when the
bundle doesn't contain this commit, the line is not present.
Øyvind
^ permalink raw reply
* Re: Ambiguous reference weirdness
From: Jeff King @ 2012-02-22 20:38 UTC (permalink / raw)
To: Phil Hord; +Cc: git, Ramkumar Ramachandra, Junio C Hamano, Jonathan Nieder
In-Reply-To: <CABURp0r+3gOQ3rq_ubv=uEoU=XmtYs=etfT4W2Lb9M0LBrikWg@mail.gmail.com>
On Wed, Feb 22, 2012 at 09:48:23AM -0500, Phil Hord wrote:
> > What is 1147? Is it supposed to be a partial sha1, or is it a ref you
> > have?
>
> 1147 was a typo. It was a Gerritt changeset ID I forgot to expand.
> When I type "git cherry-pick 1147<TAB>", autocompletion expands this
> for me to "git cherry-pick origin/changes/47/1147/1". Except in this
> case I misfired the TAB and got the weird "BUG:" report. But I didn't
> see the same problem with other invalid refs, so I went searching for
> the variants and to see what caused it.
Ah. It's a little annoying that Gerrit names refs that look kind of
like partial sha1s. But I guess most of the time it's not that big a
deal (it is only because you were using the partial Gerrit ref with tab
completion). And I don't think we're about to change how Gerrit names
things. :)
> > Have you looked at the object that it resolves to? I suspect it is the
> > partial sha1 of a non-commit object. E.g.:
>
> All of these examples were run in current git.git, so you can try them
> yourself if needed. But I did figure out that 1147 resolves to a
> blob, and that's apparently the difference between these three:
Yeah. I figured that after reading more, but didn't go back and revise
the first part of my message. I was able to easily get the same results
as you (actually, in my git.git, 1147 is ambiguous because I happen to
have some extra refs, but it was easy to replicate with a fresh clone).
> $ git cherry-pick 1147
> fatal: BUG: expected exactly one commit from walk
>
> $ git cherry-pick 1146
> error: short SHA1 1146 is ambiguous.
> error: short SHA1 1146 is ambiguous.
> fatal: ambiguous argument '1146': unknown revision or path not in the
> working tree.
> Use '--' to separate paths from revisions
>
> $ git cherry-pick 114333
> fatal: ambiguous argument '114333': unknown revision or path not in
> the working tree.
> Use '--' to separate paths from revisions
>
> I consider the first two responses to be UI bugs. The second one is
> minor (the twice-reported error message), and the first one is pretty
> rude. I would expect all three to report the same conclusion,
> "fatal: ambiguous argument 'XXXXX': unknown revision or path not in
> the working tree." But the first one doesn't.
Right. Because cherry-pick's logic is:
if the arguments do not resolve to any objects at all
complain of unknown revision
if the resolved object is not a single commit
die of BUG
And I think you just want to collapse those two conditions into a single
conditional, which seems reasonable (the error message does say "unknown
revision", not "unknown object". It's a little more complicated than
that, because you are crossing a boundary between reusable library code
and cherry-pick specific code.
As for the doubled "ambiguous" warning, I'm not sure what the cause is
for that. I suspect it is because cherry-pick tries to parse one way (as
a revision walk specifier, like "a..b"), and then parses again (as a
single commit) when that fails, due to historical reasons. Obviously
it would be nice to see that improved, but I suspect it will be annoying
to do so; the error message is emitted by a low-level, and cherry-pick
has no idea that it has happened (it only sees "this didn't result in
parsing anything").
> Thanks. I'm not familiar with the {tree} syntax -- in fact I'd like
> to find a dictionary for all the reference spelling variants -- but
> this is elucidating.
See "git help rev-parse", section "Specifying Revisions".
> > In the cherry-pick case, the code is checking the right thing, but the
> > message is horrible. It is not a bug, but merely unexpected input, and
> > it should provide a usage message.
>
> Bug is too strong a word in one sense, but from the user perspective I
> consider this "horrible message" a bug.
Sorry, I meant git is wrong to use the word "BUG". For us, die("BUG:
...") is the equivalent of an assert. It means something totally
unexpected happened that should never happen, and therefore there is a
bug in git. But this is not a bug in git (well, it is, but not that
kind). It is a totally reasonable and expected malformed input that git
should diagnose and report correctly.
So it is a bug that git says "BUG". It should say "you gave me objects,
but they are not revisions" or something similar.
> > I think checkout has the same "is this a path or a revision" ambiguity
> > to resolve. But rather than be explicit that you might have meant "114"
> > as a tree, the error message assumes you meant a path. That might be
> > worth improving, similar to the above example.
> >
> > Again, you can disambiguate with:
> >
> > $ git checkout -- 1147
> > error: pathspec '1147' did not match any file(s) known to git.
> >
> > $ git checkout 1147 --
> > fatal: reference is not a tree: 1147
> >
> >> $ git checkout 1147
> >> fatal: reference is not a tree: 1147
>
> Yes, I understand this. This was a typo and it was ambiguous. But
> shouldn't we tell the user the same thing when encountering the same
> failure?
No, because there are really three cases here:
1. The user told us 1147 is a path.
2. The user told us 1147 is a tree.
3. The user did not tell us which, and we must guess.
We guess (1) if the name does not resolve at all, and (2) otherwise. And
the error message only indicates the particular guess we made.
So while I don't think they should all have the same error message,
there are two improvements I can see:
a. If we are guessing, and 1147 resolves but is _not_ a tree, we
could guess path instead.
b. When our guess results in an error, it would be better to be
explicit about the fact that we guessed (i.e., say "this is neither
a tree nor a path that git knows about", similar to git-log).
> > I think the outcomes are all working as intended, but the error messages
> > could stand to be improved.
>
> Yes, I agree. I only meant to complain about the error messages, not
> the results. Thanks for the discussion. I'll try to look for where
> these come from and see if they can be improved within reason.
Great. I look forward to it.
-Peff
^ 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