* Re: [PATCH 4/4] push: add 'prune' option
From: Felipe Contreras @ 2012-02-23 1:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7v1upm8jnq.fsf@alter.siamese.dyndns.org>
On Thu, Feb 23, 2012 at 2:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> +--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.
>
> I do not think it adds much useful information to mention `tmp` only once
> over what is already said by the first sentence. Also, the first sentence
> of the example does not make it clear that it is assuming a same-for-same
> mapping.
Sure, the first sentence doesn't make it clear, but it would be a
valid and obvious assumption. The second sentence makes it clear, and
the name `tmp` immediately evokes a branch that will probably be
removed.
> Coming up with a precise technical description is easy, but it is hard to
> explain to the end user in easy terms, and I commend you for attempting to
> add an example in a short single sentence, though.
>
> Perhaps spelling out the underlying assumption the example makes is the
> best we could do here without going too technical.
>
> ... For example, if you are pushing all your local branches to
> update the local branches of the remote,
Yeah, but that's 'git push --all', and that's not the common
operation--'git push' is. So that's what I presumed the reader would
assume, and it really doesn't make a difference as to what will
follow:
> `tmp` branch will be
> removed from the remote if you removed your `tmp` branch locally.
This reuses the name `tmp`, which seems to be your objective, but it
doesn't explain _why_ it would remove `tmp`; is it because `tmp` is
the upstream branch, or is it because it has the same name?
> If you are pushing all your local branches on your laptop to a
> repository on your desktop machine under `refs/remotes/laptop/`
> hierarchy to back them up, `refs/remotes/laptop/tmp` is removed
> from the remote if you no longer have the branch `tmp` on your
> laptop.
Unfortunately, I as a reader have trouble understanding this. More
specifically I have trouble understanding where `refs/remotes/laptop/`
is coming from, and what it is meaning. I have always pictured
`refs/remotes` as something that 'git fetch' updates, and always from
the relevant repository. While eventually I could understand what this
thing is doing, it took me more than one read, and I had to read
slowly, and even then it seems completely non-standard to me.
I think a synthetic example, like `refs/heads/*:refs/tmp/*`, is much
easier to understand because it doesn't mess up with any established
refs, and also has the advantage that it shows the relevant refspec,
which is useful for people that are not familiar with refspecs, and in
fact, people could try it out without messing with their current refs.
>> 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),
>> };
>
> Lose the ',' at the end, for the same reason why deleted line did not have
> one.
And why is that? Isn't git c99? That comma would only ensure that the
next patch that touches this would be two lines instead of one.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCHv3 3/5] tag --exclude option
From: Junio C Hamano @ 2012-02-23 1:00 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, peff, jasampler, pclouds
In-Reply-To: <20120223002215.GE2410@tgrennan-laptop>
Tom Grennan <tmgrennan@gmail.com> writes:
> On Tue, Feb 21, 2012 at 10:33:29PM -0800, Junio C Hamano wrote:
>
>>As you do not specify what kind of pattern matching is done to this
>>exclude pattern, it is important to use the same logic between positive
>>and negative ones to give users a consistent UI. Unfortunately we use
>>fnmatch without FNM_PATHNAME for positive ones, so this exclude pattern
>>needs to follow the same semantics to reduce confusion.
>>
>>This comment applies to all the patches in this series to add this option
>>to existing commands that take the positive pattern.
>
> OK, should I also describe the --no-exclude option?
If you support it, yes. What does it do?
--no-exclude::
Cancels all the `--exclude` options given so far on the command line.
perhaps?
^ permalink raw reply
* Re: [PATCHv3 1/5] refs: add match_pattern()
From: Tom Grennan @ 2012-02-23 0:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, jasampler, pclouds
In-Reply-To: <7v62ey8ktp.fsf@alter.siamese.dyndns.org>
On Wed, Feb 22, 2012 at 04:17:22PM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> Yes, I tried to minimize change but the conversion of argv's to
>> string_list's won't add that much.
>
>How about _not_ using string_list? After all, string_list is not just a
>collection of strings, but is a table to hold strings with attributes. I
>thought argv_array is more appropriate abstraction for the purpose of your
>patch.
OK, It looks like I should also add a common parse_opt_argv_array() to
parse-options-cb. Of course that would be in a separate, dependent
patch.
>>> ref_match_pattern("refs/heads/master",
>>> ["maste?", NULL],
>>> ["refs/heads/", NULL],
>>> (REF_MATCH_FNM_PATH|REF_MATCH_LEADING));
>>>
>>>Note that the above "ref_match_pattern()" gives the same "flags" for the
>>>call to match_one() for elements in both positive and negative array and
>>>it is very deliberate. See review comment to [3/5] for the reasoning.
>>
>> OK, I think that I understand, but please confirm, you'd expect no output in
>> the above example, right?
>
>"maste?" would match with FNM_PATHNAME with "refs/heads/master" but
>the negative "refs/heads/" matches with it, so yeah, I expect that the
>function would return false.
thanks, that's a good test case.
--
TomG
^ permalink raw reply
* how do I review gitk release notes
From: Neal Kreitzinger @ 2012-02-23 0:47 UTC (permalink / raw)
To: git
What is the best way to get the corresponding gitk release notes for git
releases (so I can see the detailed gitk release notes behind the git
release note "various gitk enhancements and fixes")?
v/r,
neal
^ permalink raw reply
* Re: [PATCH 4/4] push: add 'prune' option
From: Junio C Hamano @ 2012-02-23 0:42 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <1329950621-21165-5-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> +--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.
I do not think it adds much useful information to mention `tmp` only once
over what is already said by the first sentence. Also, the first sentence
of the example does not make it clear that it is assuming a same-for-same
mapping.
Coming up with a precise technical description is easy, but it is hard to
explain to the end user in easy terms, and I commend you for attempting to
add an example in a short single sentence, though.
Perhaps spelling out the underlying assumption the example makes is the
best we could do here without going too technical.
... For example, if you are pushing all your local branches to
update the local branches of the remote, `tmp` branch will be
removed from the remote if you removed your `tmp` branch locally.
If you are pushing all your local branches on your laptop to a
repository on your desktop machine under `refs/remotes/laptop/`
hierarchy to back them up, `refs/remotes/laptop/tmp` is removed
from the remote if you no longer have the branch `tmp` on your
laptop.
Will queue with a slight fix-ups, including this bit:
> 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),
> };
Lose the ',' at the end, for the same reason why deleted line did not have
one.
Thanks.
^ permalink raw reply
* Re: [PATCHv3 3/5] tag --exclude option
From: Tom Grennan @ 2012-02-23 0:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, jasampler, pclouds
In-Reply-To: <7vhayjbcna.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 10:33:29PM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> Example,
>> $ git tag -l --exclude "*-rc?" "v1.7.8*"
>> v1.7.8
>> v1.7.8.1
>> v1.7.8.2
>> v1.7.8.3
>> v1.7.8.4
>>
>> Which is equivalent to,
>> $ git tag -l "v1.7.8*" | grep -v \\-rc.
>> v1.7.8
>> v1.7.8.1
>> v1.7.8.2
>> v1.7.8.3
>> v1.7.8.4
>>
>> Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
>
>Having an example is a good way to illustrate your explanation, but it is
>not a substitution. Could we have at least one real sentence to describe
>what the added option *does*?
>
>This comment applies to all the patches in this series except for the
>second patch.
>
OK, I'll add the "exclude" option description from the respective man pages.
>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>> index 8d32b9a..470bd80 100644
>> --- a/Documentation/git-tag.txt
>> +++ b/Documentation/git-tag.txt
>> @@ -13,7 +13,7 @@ SYNOPSIS
>> <tagname> [<commit> | <object>]
>> 'git tag' -d <tagname>...
>> 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
>> - [<pattern>...]
>> + [--exclude <pattern>] [<pattern>...]
>> 'git tag' -v <tagname>...
>>
>> DESCRIPTION
>> @@ -90,6 +90,10 @@ OPTIONS
>> --points-at <object>::
>> Only list tags of the given object.
>>
>> +--exclude <pattern>::
>> + Don't list tags matching the given pattern. This has precedence
>> + over any other match pattern arguments.
>
>As you do not specify what kind of pattern matching is done to this
>exclude pattern, it is important to use the same logic between positive
>and negative ones to give users a consistent UI. Unfortunately we use
>fnmatch without FNM_PATHNAME for positive ones, so this exclude pattern
>needs to follow the same semantics to reduce confusion.
>
>This comment applies to all the patches in this series to add this option
>to existing commands that take the positive pattern.
OK, should I also describe the --no-exclude option?
>> @@ -202,6 +206,15 @@ test_expect_success \
>> '
>>
>> cat >expect <<EOF
>> +v0.2.1
>> +EOF
>> +test_expect_success \
>> + 'listing tags with a suffix as pattern and prefix exclusion' '
>> + git tag -l --exclude "v1.*" "*.1" > actual &&
>> + test_cmp expect actual
>> +'
>
>I know you are imitating the style of surrounding tests that is an older
>parts of this script, but it is an eyesore. More modern tests are written
>like this:
>
> test_expect_success 'label for the test' '
> cat >expect <<-EOF &&
> v0.2.1
> EOF
> git tag -l ... >actual &&
> test_cmp expect actual
> '
>
>to avoid unnecessary backslash on the first line, and have the preparation
>of test vectore _inside_ test_expect_success. We would eventually want to
>update the older part to the newer style for consistency.
>
>Two possible ways to go about this are (1) have a "pure style" patch at
>the beginning to update older tests to a new style and then add new code
>and new test as a follow-up patch written in modern, or (2) add new code
>and new test in modern, and make a mental note to update the older ones
>after the dust settles. Adding new tests written in older style to a file
>that already has mixed styles is the worst thing you can do.
>
>This comment applies to all the patches in this series with tests.
I'd prefer, (1) precede each "--exclude" patch with a "pure style" patch
to update the respective tests. However, since this will result in a
lot of conflict with concurrent development; I'll separate the test
patches from the code and documentation. I'll then cycle on rebasing
the style and new test patches until the development of each is
quiescent.
Thanks,
TomG
^ permalink raw reply
* Re: [PATCHv3 1/5] refs: add match_pattern()
From: Junio C Hamano @ 2012-02-23 0:17 UTC (permalink / raw)
To: Tom Grennan; +Cc: Junio C Hamano, git, peff, jasampler, pclouds
In-Reply-To: <20120222234733.GD2410@tgrennan-laptop>
Tom Grennan <tmgrennan@gmail.com> writes:
> Yes, I tried to minimize change but the conversion of argv's to
> string_list's won't add that much.
How about _not_ using string_list? After all, string_list is not just a
collection of strings, but is a table to hold strings with attributes. I
thought argv_array is more appropriate abstraction for the purpose of your
patch.
>> ref_match_pattern("refs/heads/master",
>> ["maste?", NULL],
>> ["refs/heads/", NULL],
>> (REF_MATCH_FNM_PATH|REF_MATCH_LEADING));
>>
>>Note that the above "ref_match_pattern()" gives the same "flags" for the
>>call to match_one() for elements in both positive and negative array and
>>it is very deliberate. See review comment to [3/5] for the reasoning.
>
> OK, I think that I understand, but please confirm, you'd expect no output in
> the above example, right?
"maste?" would match with FNM_PATHNAME with "refs/heads/master" but
the negative "refs/heads/" matches with it, so yeah, I expect that the
function would return false.
^ permalink raw reply
* Re: [RFC/PATCH 3/3] push: add 'prune' option
From: Junio C Hamano @ 2012-02-22 23:54 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <CAMP44s0DnVfd_Ged5rnhM4BF1jEtm4jzyq9sJAoEtQ-0CM5_aA@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> ... Of course, using --prune
> --mirror is the same as --mirror.
Sounds good. Thanks.
^ permalink raw reply
* Re: [PATCH v2] contrib: added git-diffall
From: Junio C Hamano @ 2012-02-22 23:48 UTC (permalink / raw)
To: Tim Henigan; +Cc: git, davvid, stefano.lattarini
In-Reply-To: <1329948749-5908-1-git-send-email-tim.henigan@gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> 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.
We encourage our log messages to describe the problem first and then
present solution to the problem, so I would update the above perhaps like
this:
The 'git difftool' command lets the user to use an external tool
to view diffs, but it runs the tool for one file at the time. This
makes it tedious to review a change that spans multiple files.
The "git-diffall" script instead prepares temporary directories
with preimage and postimage files, and launches a single instance
of an external diff tool to view the differences in them.
diff.tool or merge.tool configuration variable is used to specify
what external tool is used.
I am wondering if reusing "diff.tool" or "merge.tool" is a good idea,
though.
I guess that it is OK to assume that any external tool that can compare
two directories MUST be able to compare two individual files, and if that
is true, it is perfectly fine to reuse the configuration. But if an
external tool "frobdiff" that can compare two directories cannot compare
two individual files, it will make it impossible for the user to run "git
difftool" if diff.tool is set to "frobdiff" to use with "diffall".
Another thing that comes to my mind is if a user has an external tool that
can use "diffall", is there ever a situation where the user chooses to use
"difftool" instead, to go files one by one. I cannot offhand imagine any.
Perhaps a longer term integration plan may be to lift the logic from this
script and make it part of "difftool", and then add a boolean variable
"difftool.<tool>.canCompareDirectory", without adding "git diffall" as a
subcommand. The user can still run "git difftool", and when the external
tool can compare two directories, the code to populate temporary directory
(and to set the trap to clean after itself) taken from this tool will run
inside "git difftool" frontend and then the external tool to compare the
two directories is spawned.
I also think that in a yet longer term, if this mode of "instantiate two
directories to be compared, and let the external tool do the comparison"
proves useful, almost all the "interesting" work done in this script
should be made unnecessary by adding an updated "external diff interface"
on the core side, so that nobody has to hurt his brain to implement an
error-prone command line parsing logic.
In other words, this statement cannot stay true:
> +This script is compatible with all the forms used to specify a
> +range of revisions to diff:
without updating the script every time underlying "git diff" gains new way
of comparing things. If we move the "prepare two directories, and point
an external tool at them" logic to the core, we do not have to worry about
it at all.
Besides, I do not think the script covers all the forms; "git diff -R"
support is totally missing.
But that is all two steps in the future.
> +# 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
> +}
It would not withstand malicious attacks, but doing
tmp=/tmp/git-diffall-tmp.$$
would at least protect you from accidental name crashes better in the
fallback codepath.
> +
> +trap 'rm -rf "$tmp" 2>/dev/null' EXIT
Do you need to suppress errors, especially when you are running "rm -rf"
with the 'f' option?
> +mkdir -p "$tmp"
> +
> +left=
> +right=
> +paths=
> +path_sep=
> +compare_staged=
> +common_ancestor=
> +left_dir=
> +right_dir=
> +diff_tool=
> +copy_back=
You can write multiple assignment on a line to save vertical space if you
want to, and the initialization sequence like this is a good place to do
so.
> +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
> + ;;
What if your command line ends with -x without $2?
Don't you want to match "-t/--tool" that "difftool" already uses?
> + --)
> + 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
> + ;;
Strictly speaking, that is not just a common_ancestor but is a merge_base,
which is a common ancestor none of whose children is a common ancestor.
> + *..*)
> + 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
Hrm, so "diffall HEAD~2 Documentation/" is not the way to compare the
contents of the Documentation/ directory between the named commit and
the working tree, like "diff HEAD~2 Documentation/" does.
That is not a show-stopper (a double-dash is an easy workaround), but
it is worth pointing out.
> + ;;
> + 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
And this will not work with pathspec that have $IFS characters. If we
really wanted to we could do that by properly quoting "$1" when you build
$paths and then use eval when you run "git diff" here (look for 'sq' and
'eval' in existing scripts, e.g. "git-am.sh", if you are interested).
Also you may want to write filelist using -z format to protect yourself
from paths that contain LF, but that would require the loop "while read
name" to be rewritten.
> +# Exit immediately if there are no diffs
> +if test ! -s "$tmp/filelist"
> +then
> + exit 0
> +fi
Ok, you have trap set already so $tmp will disappear with this exit ;-)
> +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
I actually wondered why $right_dir needs to be populated with a copy in
the first place (if you do not copy but give the working tree itself to
the external tool, you do not even have to copy back).
I know the answer to the question, namely, "because the external tool
thinks files that are not in $left_dir are added files", but if you can
find a way to tell the external tool to ignore new files (similar to how
"diff -r" without -N works), running the tool with temporary left_dir and
the true workdir as right_dir would be a lot cleaner solution to the
problem.
> +# 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"
"while read -r name" might make this slightly more robust; even though
this loses leading and trailing whitespaces in filenames, we probably
can get away without worrying about them.
> +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)
> + }
What is this "--ignore-failed-read" about? Not reporting unreadable as an
error smells really bad.
If you require GNUism in your tar usage, this should be made configurable
so that people can use alternative names (some systems come with "tar"
that is POSIX and "gtar" that is GNU).
> +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
This will copy new files created in $right_dir. Is that intended?
^ permalink raw reply
* Re: [PATCHv3 1/5] refs: add match_pattern()
From: Tom Grennan @ 2012-02-22 23:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, jasampler, pclouds
In-Reply-To: <7vobsrbcny.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 10:33:05PM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> +static int match_path(const char *name, const char *pattern, int nlen)
>> +{
>> + int plen = strlen(pattern);
>> +
>> + return ((plen <= nlen) &&
>> + !strncmp(name, pattern, plen) &&
>> + (name[plen] == '\0' ||
>> + name[plen] == '/' ||
>> + pattern[plen-1] == '/'));
>> +}
>
>This is a counterpart to the tail match found in ls-remote, so we would
>want to call it with a name that makes it clear this is a leading path
>match not just "path" match. Perhaps match_leading_path() or something.
OK
>> +int match_pattern(const char *name, const char **match,
>> + struct string_list *exclude, int flags)
>> +{
>> + int nlen = strlen(name);
>> +
>> + if (exclude) {
>> + struct string_list_item *x;
>> + for_each_string_list_item(x, exclude) {
>> + if (!fnmatch(x->string, name, 0))
>> + return 0;
>> + }
>> + }
>> + if (!match || !*match)
>> + return 1;
>> + for (; *match; match++) {
>> + if (flags == FNM_PATHNAME)
>> + if (match_path(name, *match, nlen))
>> + return 1;
>> + if (!fnmatch(*match, name, flags))
>> + return 1;
>> + }
>> + return 0;
>> +}
>
>As an API for a consolidated and generic function, the design needs a bit
>more improving, I would think.
>
> - The name match_pattern() was OK for a static function inside a single
> file, but it is way too vague for a global function. This is to match
> refnames, so I suspect there should at least be a string "ref_"
> somewhere in its name.
OK
> - You pass "flags" argument, so that later we _could_ enhance the
> implementation to cover needs for new callers, but alas, it uses its
> full bits to express only one "do we do FNM_PATHNAME or not?" bit of
> information, so essentially "flags" does not give us any expandability.
I agree.
> - Is it a sane assumption that a caller that asks FNM_PATHNAME will
> always want match_path() semantics, too? Aren't these two logically
> independent?
Yes, these should be ligically independent although the current use has
combined them.
> - Is it a sane assumption that a caller that gives an exclude list will
> want neither FNM_PATHNAME semantics nor match_path() semantics?
I'm not sure. I tried using FNM_PATHNAME with both exclusion and match
patterns of git-for-each-ref but I couldn't get it to do something like
this:
git for-each-ref ... --exclude '*HEAD' refs/remotes/
I don't remember if this worked,
git for-each-ref ... --exclude HEAD refs/remotes/
Now I see how an implicit TRAILING match would be useful,
git for-each-ref ... --exclude /HEAD refs/remotes/
Where git-for-each-ref uses this flag:
REF_MATCH_LEADING | REF_MATCH_TRAILING | REF_MATCH_FNM_PATH
I'll experiment with this more.
> - Positive patterns are passed in "const char **match", and negative ones
> are in "struct string_list *". Doesn't the inconsistency strike you as
> strange?
Yes, I tried to minimize change but the conversion of argv's to
string_list's won't add that much.
>Perhaps like...
>
>#define REF_MATCH_LEADING 01
>#define REF_MATCH_TRAILING 02
>#define REF_MATCH_FNM_PATH 04
>
>static int match_one(const char *name, size_t namelen, const char *pattern,
> unsigned flags)
>{
> if ((flags & REF_MATCH_LEADING) &&
> match_leading_path(name, pattern, namelen))
> return 1;
> if ((flags & REF_MATCH_TRAILING) &&
> match_trailing_path(name, pattern, namelen))
> return 1;
> if (!fnmatch(pattern, name,
> (flags & REF_MATCH_FNM_PATH) ? FNM_PATHNAME : 0))
> return 1;
> return 0;
>}
>
>int ref_match_pattern(const char *name,
> const char **pattern, const char **exclude, unsigned flags)
>{
> size_t namelen = strlen(name);
> if (exclude) {
> while (*exclude) {
> if (match_one(name, namelen, *exclude, flags))
> return 0;
> exclude++;
> }
> }
> if (!pattern || !*pattern)
> return 1;
> while (*pattern) {
> if (match_one(name, namelen, *pattern, flags))
> return 1;
> pattern++;
> }
> return 0;
>}
>
>and then the caller could do something like
>
> ref_match_pattern("refs/heads/master",
> ["maste?", NULL],
> ["refs/heads/", NULL],
> (REF_MATCH_FNM_PATH|REF_MATCH_LEADING));
>
>Note that the above "ref_match_pattern()" gives the same "flags" for the
>call to match_one() for elements in both positive and negative array and
>it is very deliberate. See review comment to [3/5] for the reasoning.
OK, I think that I understand, but please confirm, you'd expect no output in
the above example, right?
--
TomG
^ permalink raw reply
* Re: [RFC/PATCH 3/3] push: add 'prune' option
From: Felipe Contreras @ 2012-02-22 23:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <CAMP44s3+XCM1E_AtW1yGifmGoGSkFSpSTaFbbMffz+hmUzWahw@mail.gmail.com>
On Wed, Feb 22, 2012 at 10:43 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
Actually, after writing the patch for the documentation I realized
this would be difficult to describe: you can use --all --prune, but
not --mirror --prune, and the documentation currently has '[--all |
--mirror | --tags]'. So I decided to make it orthogonal, you can use
--prune with --all, --tags, *and* --mirror. Of course, using --prune
--mirror is the same as --mirror.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: Manual hunk edit mode + emacs + ^G == garbage
From: Matt McClure @ 2012-02-22 21:43 UTC (permalink / raw)
To: git
In-Reply-To: <EDCC7CB3-4DFF-45B8-9E23-E12045CC29D7@sb.org>
Kevin Ballard <kevin <at> sb.org> writes:
> I'm still very much interested in finding a solution to why ^G kills
> emacs when it's invoked by git. As I said earlier, it appears that emacs
> shares the same process group with the perl process that called it, even
> though Andreas Schwab says it puts itself into its own process group and
> invoking emacs from the shell does just that. Does anyone know why this
> might be happening?
Kevin,
I was having the same problem you described on Mac OS X Lion and found this
old thread. A workaround that works for me is:
~/bin/emacs.sh:
#!/bin/bash -i
emacs "$@"
-- cut --
git config --global core.editor ~/bin/emacs.sh
Matt
^ permalink raw reply
* [PATCH 1/4] remote: use a local variable in match_push_refs()
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>
So that we can reuse src later on. No functional changes.
Will be useful in next patches.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
remote.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/remote.c b/remote.c
index 73a3809..55d68d1 100644
--- a/remote.c
+++ b/remote.c
@@ -1157,7 +1157,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
int send_mirror = flags & MATCH_REFS_MIRROR;
int errs;
static const char *default_refspec[] = { ":", NULL };
- struct ref **dst_tail = tail_ref(dst);
+ struct ref *ref, **dst_tail = tail_ref(dst);
if (!nr_refspec) {
nr_refspec = 1;
@@ -1167,14 +1167,14 @@ int match_push_refs(struct ref *src, struct ref **dst,
errs = match_explicit_refs(src, *dst, &dst_tail, rs, nr_refspec);
/* pick the remainder */
- for ( ; src; src = src->next) {
+ for (ref = src; ref; ref = ref->next) {
struct ref *dst_peer;
const struct refspec *pat = NULL;
char *dst_name;
- if (src->peer_ref)
+ if (ref->peer_ref)
continue;
- pat = check_pattern_match(rs, nr_refspec, src);
+ pat = check_pattern_match(rs, nr_refspec, ref);
if (!pat)
continue;
@@ -1184,13 +1184,14 @@ int match_push_refs(struct ref *src, struct ref **dst,
* including refs outside refs/heads/ hierarchy, but
* that does not make much sense these days.
*/
- if (!send_mirror && prefixcmp(src->name, "refs/heads/"))
+ if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
continue;
- dst_name = xstrdup(src->name);
+ dst_name = xstrdup(ref->name);
+
} else {
const char *dst_side = pat->dst ? pat->dst : pat->src;
- if (!match_name_with_pattern(pat->src, src->name,
+ if (!match_name_with_pattern(pat->src, ref->name,
dst_side, &dst_name))
die("Didn't think it matches any more");
}
@@ -1211,9 +1212,9 @@ int match_push_refs(struct ref *src, struct ref **dst,
/* Create a new one and link it */
dst_peer = make_linked_ref(dst_name, &dst_tail);
- hashcpy(dst_peer->new_sha1, src->new_sha1);
+ hashcpy(dst_peer->new_sha1, ref->new_sha1);
}
- dst_peer->peer_ref = copy_ref(src);
+ dst_peer->peer_ref = copy_ref(ref);
dst_peer->force = pat->force;
free_name:
free(dst_name);
--
1.7.9.1
^ permalink raw reply related
* [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
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