* Re: [PATCH] Completion: Add support for --submodule=diff
From: Junio C Hamano @ 2017-01-30 23:03 UTC (permalink / raw)
To: Peter Law; +Cc: git, Jacob Keller, szeder
In-Reply-To: <CAKoneT+Bn+MdbeNnPJsu23rbLCZ=jxADNVtpNefw9zNYMq26dA@mail.gmail.com>
Peter Law <peterjclaw@gmail.com> writes:
>> Teach git-completion.bash about the 'diff' option to 'git diff
>> --submodule=', which was added in Git 2.11.
>
> I posted this patch back in December, but I've not heard anything. I'm
> sure as maintainers you're all quite busy, but I was wondering how
> long it usually takes to get a response to patches? (also whether I'd
> gotten some part of the submission process wrong?)
When there is clear "subsystem maintainer(s)" in the area, I try to
refrain from commenting until they speak up, and completion scripts
are one of these areas. I usually ping them after a few days but in
December with holidays and things people are usually slow, and so
was I X-<.
Will pick it up. Thanks for a reminder; you absolutely did the
right thing (i.e. sending it out with people who are likely to know
about the area CC'ed, waiting for a while and then sending a
reminder).
^ permalink raw reply
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Junio C Hamano @ 2017-01-30 22:57 UTC (permalink / raw)
To: cornelius.weig; +Cc: peff, git
In-Reply-To: <xmqq37g0us5p.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index bfe685c..81ea2ed 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>> const char *old_desc, *reflog_msg;
>> if (opts->new_branch) {
>> if (opts->new_orphan_branch) {
>> - if (opts->new_branch_log && !log_all_ref_updates) {
>> + const char *refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
>> ...
>> if (ret) {
>> fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
>> opts->new_orphan_branch, err.buf);
>
> Here you need to have another free(), as this block makes an early
> return and you end up leaking refname.
I am building with the attached patch squashed on top.
The extra free(refname) is to plug the leak I pointed out, and the
type of refname is no longer const, because "const char *" cannot be
free()d without casting, and in this codepath I do not see a reason
to mark it as const.
When queued on top of 4e59582ff7 ("Seventh batch for 2.12",
2017-01-23), however, this fails t2017#9 (orphan with -l makes
reflog when core.logAllRefUpdates = false).
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 81ea2eda99..e1a60fd8ea 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -612,7 +612,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
const char *old_desc, *reflog_msg;
if (opts->new_branch) {
if (opts->new_orphan_branch) {
- const char *refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
+ char *refname;
+
+ refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
if (opts->new_branch_log && should_autocreate_reflog(refname)) {
int ret;
struct strbuf err = STRBUF_INIT;
@@ -622,6 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
opts->new_orphan_branch, err.buf);
strbuf_release(&err);
+ free(refname);
return;
}
strbuf_release(&err);
^ permalink raw reply related
* Re: [PATCH v4 0/5] urlmatch: allow wildcard-based matches
From: Junio C Hamano @ 2017-01-30 22:52 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Patrick Steinhardt, Philip Oakley
In-Reply-To: <xmqqy3xstdh9.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Patrick Steinhardt <patrick.steinhardt@elego.de> writes:
>
>> - I realized that with my patches, "ranking" of URLs was broken.
>> Previously, we've always taken the longest matching URL. As
>> previously, only the user and path could actually differ, only
>> these two components were used for the comparison. I've
>> changed this now to also include the host part so that URLs
>> with a longer host will take precedence. This resulted in a
>> the patch 4.
>
> Good thinking. I was wondering about this, too.
>
> Thanks. Will read it through and replace.
Ugh. When applied on top of 4e59582ff7 ("Seventh batch for 2.12",
2017-01-23), Git fails its self-test in t5551 #31 (I do not run with
EXPENSIVE so some tests liks #30 are skipped, if it matters).
^ permalink raw reply
* Re: [PATCH] checkout: convert post_checkout_hook() to struct object_id
From: brian m. carlson @ 2017-01-30 22:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: René Scharfe, Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301359500.3469@virtualbox>
[-- Attachment #1: Type: text/plain, Size: 456 bytes --]
On Mon, Jan 30, 2017 at 02:01:12PM +0100, Johannes Schindelin wrote:
> Hi René,
>
> On Sat, 28 Jan 2017, René Scharfe wrote:
>
> > Signed-off-by: Rene Scharfe <l.s.r@web.de>
>
> These three SHA-1 -> OID patches all appear correct to me.
I concur. These look good.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH 4/5] diff: use SWAP macro
From: Junio C Hamano @ 2017-01-30 22:22 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Johannes Schindelin
In-Reply-To: <84944ecd-d14e-b5e9-7566-9ab2b68c02fb@web.de>
René Scharfe <l.s.r@web.de> writes:
> Use the macro SWAP to exchange the value of pairs of variables instead
> of swapping them manually with the help of a temporary variable. The
> resulting code is shorter and easier to read.
>
> The two cases were not transformed by the semantic patch swap.cocci
> because it's extra careful and handles only cases where the types of all
> variables are the same
Ah, that explains my "huh?" in 3/5; thanks.
^ permalink raw reply
* Re: [PATCH 3/5] use SWAP macro
From: Junio C Hamano @ 2017-01-30 22:22 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Johannes Schindelin
In-Reply-To: <187c2b39-40cf-7e07-b489-d40cdf5f9145@web.de>
René Scharfe <l.s.r@web.de> writes:
> if (tree2->flags & UNINTERESTING) {
> - struct object *tmp = tree2;
> - tree2 = tree1;
> - tree1 = tmp;
> + SWAP(tree2, tree1);
A human would have written this SWAP(tree1, tree2).
Not that I think such a manual fix-up should be made in _this_
patch, which may end up mixing mechanical conversion (which we may
want to keep reproducible) and hand tweaks. But this swapped swap
reads somewhat silly.
> diff --git a/diff-no-index.c b/diff-no-index.c
> index f420786039..1ae09894d7 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -186,9 +186,8 @@ static int queue_diff(struct diff_options *o,
>
> if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
> unsigned tmp;
> - const char *tmp_c;
> tmp = mode1; mode1 = mode2; mode2 = tmp;
> - tmp_c = name1; name1 = name2; name2 = tmp_c;
> + SWAP(name1, name2);
Curious that mode swapping is left for a later iteration.
> diff --git a/diff.c b/diff.c
> index f08cd8e033..9de1ba264f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5118,13 +5118,11 @@ void diff_change(struct diff_options *options,
>
> if (DIFF_OPT_TST(options, REVERSE_DIFF)) {
> unsigned tmp;
> - const unsigned char *tmp_c;
> - tmp = old_mode; old_mode = new_mode; new_mode = tmp;
> - tmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;
> + SWAP(old_mode, new_mode);
> + SWAP(old_sha1, new_sha1);
> tmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;
> new_sha1_valid = tmp;
So is this one.
> diff --git a/merge-recursive.c b/merge-recursive.c
> ...
> - tmp = ren2;
> - ren2 = ren1;
> - ren1 = tmp;
> + SWAP(ren2, ren1);
A human would have written this SWAP(ren1, ren2).
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Brandon Williams @ 2017-01-30 22:21 UTC (permalink / raw)
To: René Scharfe
Cc: Johannes Schindelin, Johannes Sixt, Git List, Junio C Hamano
In-Reply-To: <77098ac8-1084-a5c4-a5e6-fb382e3dc3a0@web.de>
On 01/30, René Scharfe wrote:
> Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:
> >It is curious, though, that an
> >expression like "sizeof(a++)" would not be rejected.
>
> Clang normally warns about something like this ("warning: expression
> with side effects has no effect in an unevaluated context
> [-Wunevaluated-expression]"), but not if the code is part of a
> macro. I don't know if that's intended, but it sure is helpful in
> the case of SWAP.
>
> >Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?
>
> That might be a valid expectation, but GCC says "error: lvalue
> required as unary '&' operand" and clang puts it "error: cannot take
> the address of an rvalue of type".
>
> René
Perhaps we could disallow a side-effect operator in the macro. By
disallow I mean place a comment at the definition to the macro and
hopefully catch something like that in code-review. We have the same
issue with the `ALLOC_GROW()` macro.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-01-30 22:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701302158110.3469@virtualbox>
Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:
> It is curious, though, that an
> expression like "sizeof(a++)" would not be rejected.
Clang normally warns about something like this ("warning: expression
with side effects has no effect in an unevaluated context
[-Wunevaluated-expression]"), but not if the code is part of a macro. I
don't know if that's intended, but it sure is helpful in the case of SWAP.
> Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?
That might be a valid expectation, but GCC says "error: lvalue required
as unary '&' operand" and clang puts it "error: cannot take the address
of an rvalue of type".
René
^ permalink raw reply
* Re: [PATCH v4 0/5] urlmatch: allow wildcard-based matches
From: Junio C Hamano @ 2017-01-30 22:00 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Patrick Steinhardt, Philip Oakley
In-Reply-To: <cover.1485512626.git.patrick.steinhardt@elego.de>
Patrick Steinhardt <patrick.steinhardt@elego.de> writes:
> - I realized that with my patches, "ranking" of URLs was broken.
> Previously, we've always taken the longest matching URL. As
> previously, only the user and path could actually differ, only
> these two components were used for the comparison. I've
> changed this now to also include the host part so that URLs
> with a longer host will take precedence. This resulted in a
> the patch 4.
Good thinking. I was wondering about this, too.
Thanks. Will read it through and replace.
^ permalink raw reply
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Junio C Hamano @ 2017-01-30 21:58 UTC (permalink / raw)
To: cornelius.weig; +Cc: peff, git
In-Reply-To: <20170127100948.29408-2-cornelius.weig@tngtech.com>
cornelius.weig@tngtech.com writes:
> Notes:
> Changes wrt v2:
>
> - change wording in commit message s/do not typically/are not meant to/;
> - in update_refs_for_switch move refname to the enclosing block, so that
> should_autocreate_reflog has access. Thanks Junio for spotting this
> potential bug early :)
> - add test that asserts reflogs are created for tags if
> logAllRefUpdates=always. The case with logAllRefUpdates=true is IMHO already
> covered by the default case. To make that clearer, I explicitly added
> logAllRefUpdates=true.
These look all sensible. Especially thanks for reordering the code
to feed the real refname for the new branch in the "checkout"
codepath.
> When writing the test for git-tag, I realized that the option
> --no-create-reflog to git-tag does not take precedence over
> logAllRefUpdate=always. IOW the setting cannot be overridden on the command
> line. Do you think this is a defect or would it not be desirable to have this
> feature anyway?
"--no-create-reflog" should override the configuration set to "true"
or "always". Also "--create-reflog" should override the
configuration set to "false".
If the problem was inherited from the original code before your
change (e.g. you set logAllRefUpdates to true and then did
"update-ref --no-create-reflog refs/heads/foo". Does the code
before your change ignore the command lne option and create a reflog
for the branch?), then it would be ideal to fix the bug before this
series as a preparatory fix. If the problem was introduced by this
patch set, then we would need a fix not to introduce it ;-)
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfe685c..81ea2ed 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
> const char *old_desc, *reflog_msg;
> if (opts->new_branch) {
> if (opts->new_orphan_branch) {
> - if (opts->new_branch_log && !log_all_ref_updates) {
> + const char *refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
> + if (opts->new_branch_log && should_autocreate_reflog(refname)) {
> int ret;
> - char *refname;
> struct strbuf err = STRBUF_INIT;
>
> - refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
> ret = safe_create_reflog(refname, 1, &err);
> - free(refname);
> if (ret) {
> fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
> opts->new_orphan_branch, err.buf);
Here you need to have another free(), as this block makes an early
return and you end up leaking refname.
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 1cfa8a2..1bf622d 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -71,6 +71,7 @@ test_expect_success 'creating a tag for an unknown revision should fail' '
>
> # commit used in the tests, test_tick is also called here to freeze the date:
> test_expect_success 'creating a tag using default HEAD should succeed' '
> + test_config core.logAllRefUpdates true &&
> test_tick &&
> echo foo >foo &&
> git add foo &&
This change is to make sure that 'true' does not affect tags (but
'always' does as seen in the later new test)? I am just double
checking, not objecting.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-01-30 21:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301806591.3469@virtualbox>
Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:
> So I tried to verify that Visual C optimizes this well, and oh my deity,
> this was not easy. In Debug mode, it does not optimize, i.e. the memcpy()
> will be called, even for simple 32-bit integers. In Release mode, Visual
> Studio's defaults turn on "whole-program optimization" which means that
> the only swapping that is going on in the end is that the meaning of two
> registers is swapped, i.e. the SWAP() macro is expanded to... no assembler
> code at all.
>
> After trying this and that and something else, I finally ended up with the
> file-scope optimized SWAP() resulting in this assembly code:
>
> 00007FF791311000 mov eax,dword ptr [rcx]
> 00007FF791311002 mov r8d,dword ptr [rdx]
> 00007FF791311005 mov dword ptr [rcx],r8d
> 00007FF791311008 mov dword ptr [rdx],eax
This looks good.
> However, recent events (including some discussions on this mailing list
> which had unfortunate consequences) made me trust my instincts more. And
> my instincts tell me that it would be unwise to replace code that swaps
> primitive types in the straight-forward way by code that relies on
> advanced compiler optimization to generate efficient code, otherwise
> producing very suboptimal code.
I don't know how difficult it was to arrive at the result above, but I
wouldn't call inlining memcpy(3) an advanced optimization (anymore?),
since the major compilers seem to be doing that.
The SWAP in prio-queue.c seems to be the one with the biggest
performance impact. Or perhaps it's the one in lookup_object()? The
former is easier to measure, though.
Here's what I get with CFLAGS="-builtin -O2" (best of five):
$ time ./t/helper/test-prio-queue $(seq 100000 -1 1) dump >/dev/null
real 0m0.142s
user 0m0.120s
sys 0m0.020s
And this is with CFLAGS="-no-builtin -O2":
$ time ./t/helper/test-prio-queue $(seq 100000 -1 1) dump >/dev/null
real 0m0.170s
user 0m0.156s
sys 0m0.012s
Hmm. Not nice, but also not prohibitively slow.
> The commit you quoted embarrasses me, and I have no excuse for it. I would
> love to see that myswap() ugliness fixed by replacing it with a construct
> that is simpler, and generates good code even without any smart compiler.
I don't see a way to do that without adding a type parameter.
René
^ permalink raw reply
* Re: [PATCH] Completion: Add support for --submodule=diff
From: Brandon Williams @ 2017-01-30 21:43 UTC (permalink / raw)
To: Peter Law; +Cc: git, Jacob Keller, Junio C Hamano, szeder
In-Reply-To: <CAKoneT+Bn+MdbeNnPJsu23rbLCZ=jxADNVtpNefw9zNYMq26dA@mail.gmail.com>
On 01/30, Peter Law wrote:
> Hi,
>
> > Teach git-completion.bash about the 'diff' option to 'git diff
> > --submodule=', which was added in Git 2.11.
>
> I posted this patch back in December, but I've not heard anything. I'm
> sure as maintainers you're all quite busy, but I was wondering how
> long it usually takes to get a response to patches? (also whether I'd
> gotten some part of the submission process wrong?)
>
> Thanks,
> Peter
It looks like this must have just fallen through the cracks. Your patch
looks good to me and works when I test it locally.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard
From: Junio C Hamano @ 2017-01-30 21:13 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
In-Reply-To: <20170129201604.30445-2-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Don't mention git reset --hard in the documentation for git stash save.
> It's an implementation detail that doesn't matter to the end user and
> thus shouldn't be exposed to them.
Everybody understands what "reset --hard" does; it can be an
easier-to-read "short-hand" description to say "reset --hard"
instead of giving a lengthy description of what happens.
Because of that, I do not necessarily agree with the above
justification. I'll read the remainder of the series before
commenting further on the above.
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..0fc23c25ee 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -47,8 +47,9 @@ OPTIONS
>
> save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
>
> - Save your local modifications to a new 'stash', and run `git reset
> - --hard` to revert them. The <message> part is optional and gives
> + Save your local modifications to a new 'stash' and roll them
> + back both in the working tree and in the index.
"... roll them back both ..." is unclear where they are rolled back
to.
Perhaps "roll them back ... to HEAD" or something?
^ permalink raw reply
* Re: [PATCH] Completion: Add support for --submodule=diff
From: Peter Law @ 2017-01-30 21:13 UTC (permalink / raw)
To: git; +Cc: Jacob Keller, Junio C Hamano, szeder
In-Reply-To: <20161204144127.28452-1-peterjclaw@gmail.com>
Hi,
> Teach git-completion.bash about the 'diff' option to 'git diff
> --submodule=', which was added in Git 2.11.
I posted this patch back in December, but I've not heard anything. I'm
sure as maintainers you're all quite busy, but I was wondering how
long it usually takes to get a response to patches? (also whether I'd
gotten some part of the submission process wrong?)
Thanks,
Peter
^ permalink raw reply
* Re: [PATCH v2 2/4] stash: introduce push verb
From: Junio C Hamano @ 2017-01-30 21:12 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
In-Reply-To: <20170129201604.30445-3-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Introduce a new git stash push verb in addition to git stash save. The
> push verb is used to transition from the current command line arguments
> to a more conventional way, in which the message is specified after a -m
> parameter instead of being a positional argument.
I think the canonical way to express that is "... the message is
given as an argument to the -m option" (i.e. some options take an
argument, some others do not, and the "-m" takes one).
> This allows introducing a new filename argument to stash single files.
I do not want them to be "a filename argument", and I do not think
you meant them as such, either.
This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say
which subset of paths to stash (and leave others behind).
> +save_stash () {
> + push_options=
> + while test $# != 0
> + do
> + case "$1" in
> + -k|--keep-index)
> +...
> + esac
> + shift
> + done
It is a bit unfortunate that we need to duplicate the above
case/esac here. I do not know if doing it this way:
case "$1" in
--)
shift
break
;;
--help)
show_help
;;
-*)
# pass all options through to push_stash
push_options="$push_options $1"
;;
*)
break
;;
esac
and letting push_stash complain for an unknown option is easier to
maintain.
You are reversing the order of the options in the loop. Don't.
^ permalink raw reply
* Re: [PATCH v2 4/4] stash: support filename argument
From: Junio C Hamano @ 2017-01-30 21:11 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
In-Reply-To: <20170129201604.30445-5-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Add an optional filename argument to git stash push, which allows for
> stashing a single (or multiple) files.
You can give pathspec with one or more elements, so "an optional
argument" sounds too limiting.
Allow 'git stash push' to take pathspec to specify which paths
to stash.
Also retitle
stash: teach 'push' (and 'create') to honor pathspec
or something.
> @@ -56,6 +61,10 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
> only <message> does not trigger this action to prevent a misspelled
> subcommand from making an unwanted stash.
> +
> +If the paths argument is given in 'git stash push', only these files
> +are put in the new 'stash'. In addition only the indicated files are
> +changed in the working tree to match the index.
Actually the stash contains "all paths". You could say that you are
placing _modifications_ to these paths in stash, even though that is
not how Git's world model works (i.e. everything is a snapshot, and
modifications are merely difference between two successive
snapshots). A technically correct version may be:
When pathspec is given to 'git stash push', the new stash
records the modified states only for the files that match
the pathspec. The index entries and working tree files are
then rolled back to the state in HEAD only for these files,
too, leaving files that do not match the pathspec intact.
> diff --git a/git-stash.sh b/git-stash.sh
> index 5f08b43967..0072a38b4c 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -41,7 +41,7 @@ no_changes () {
> untracked_files () {
> excl_opt=--exclude-standard
> test "$untracked" = "all" && excl_opt=
> - git ls-files -o -z $excl_opt
> + git ls-files -o -z $excl_opt -- $1
Hmph, why "$1" is spelled without dq, implying that it is split at
$IFS boundary? This line alone makes me suspect that this is not
prepared to deal correctly with $IFS. Let's read on...
> @@ -59,6 +59,7 @@ create_stash () {
> stash_msg=
> untracked=
> new_style=
> + files=
> while test $# != 0
> do
> case "$1" in
> @@ -72,6 +73,12 @@ create_stash () {
> untracked="$1"
> new_style=t
> ;;
> + --)
> + shift
> + files="$@"
Isn't this the same as writing files="$*", i.e. concatenate the
multiple arguments with the first whitespace in $IFS in between?
> @@ -134,7 +141,7 @@ create_stash () {
> # Untracked files are stored by themselves in a parentless commit, for
> # ease of unpacking later.
> u_commit=$(
> - untracked_files | (
> + untracked_files $files | (
... and this lets it split at $IFS again when passing it down to the
helper. But the helper looks only at $1 so the second and subsequent
ones will be ignored altogether.
This cannot be correct, and any hunk in the remainder of the patch
that mentions $files will be incorrect for the same reason.
Is it possible to carry what the caller (and the end user) gave you
in "$@" without molesting it at all? That would mean you do not
need to introduce $files variable at all, and then the places that
do things like this:
> - create_stash -m "$stash_msg" -u "$untracked"
> + create_stash -m "$stash_msg" -u "$untracked" -- $files
can instead do
create_stash -m "$stash_msg" -u "$untracked" -- "$@"
That would allow you to work correctly with pathspec with $IFS
whitespaces in them.
^ permalink raw reply
* Re: [PATCH v2 3/4] introduce new format for git stash create
From: Junio C Hamano @ 2017-01-30 21:10 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Marc Strapetz, Jeff King, Johannes Schindelin,
Øyvind A. Holm, Jakub Narębski
In-Reply-To: <20170129201604.30445-4-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> create_stash () {
> - stash_msg="$1"
> - untracked="$2"
> + stash_msg=
> + untracked=
> + new_style=
> ...
> + while test $# != 0
> + do
> + case "$1" in
> + -m|--message)
> + shift
> + stash_msg="$1"
${1?"-m needs an argument"}
to error check "git stash create -m<Enter>"?
> + if test -z "$new_style"
> + then
> + stash_msg="$*"
> + fi
This breaks external users who do "git stash create" in the old
fashioned way, I think, but can be easily fixed with something like:
stash_msg=$1 untracked=$2
If the existing tests did not catch this, I guess there is a
coverage gap we may want to fill. Perhaps add a new test to 3903
that runs "git stash create message untracked" and makes sure it
still works?
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Johannes Schindelin @ 2017-01-30 21:03 UTC (permalink / raw)
To: Johannes Sixt; +Cc: René Scharfe, Git List, Junio C Hamano
In-Reply-To: <aa653d57-4a97-ac50-b20c-f94ed43a22fb@kdbg.org>
[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]
Hi Hannes,
On Mon, 30 Jan 2017, Johannes Sixt wrote:
> Am 30.01.2017 um 17:01 schrieb Johannes
> Schindelin:
> > On Sat, 28 Jan 2017, René Scharfe wrote:
> > > diff --git a/git-compat-util.h
> > > b/git-compat-util.h
> > > index 87237b092b..66cd466eea 100644
> > > --- a/git-compat-util.h
> > > +++ b/git-compat-util.h
> > > @@ -527,6 +527,16 @@ static inline int
> > > @@ ends_with(const char *str, const char
> > > @@ *suffix)
> > > return strip_suffix(str, suffix, &len);
> > > }
> > >
> > > +#define SWAP(a, b) do {
> > > \
> > > + void *_swap_a_ptr = &(a);
> > > \
> > > + void *_swap_b_ptr = &(b);
> > > \
> > > + unsigned char _swap_buffer[sizeof(a)];
> > > \
> > > + memcpy(_swap_buffer, _swap_a_ptr,
> > > sizeof(a)); \
> > > + memcpy(_swap_a_ptr, _swap_b_ptr,
> > > sizeof(a) + \
> > > + BUILD_ASSERT_OR_ZERO(sizeof(a)
> > > == sizeof(b))); \
> > > + memcpy(_swap_b_ptr, _swap_buffer,
> > > sizeof(a)); \
> > > +} while (0)
> > > +
> > > #if defined(NO_MMAP) ||
> > > defined(USE_WIN32_MMAP)
> >
> > It may seem as a matter of taste, or maybe
> > not: I prefer this without the
> > _swap_a_ptr
>
> The purpose of these pointers is certainly to
> avoid that the macro arguments are evaluated
> more than once.
I mistook "a" being used in sizeof(a) for breaking that assumption, but of
course a is *not* evaluated in that case. It is curious, though, that an
expression like "sizeof(a++)" would not be rejected.
Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?
Ciao,
Johannes
^ permalink raw reply
* Re: difflame
From: Junio C Hamano @ 2017-01-30 21:08 UTC (permalink / raw)
To: Jeff King; +Cc: Edmundo Carmona Antoranz, Git List
In-Reply-To: <20170128035321.yrcqwkg2fiwadxj4@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Jan 17, 2017 at 11:24:02PM -0600, Edmundo Carmona Antoranz wrote:
>
>> For a very long time I had wanted to get the output of diff to include
>> blame information as well (to see when something was added/removed).
>
> This is something I've wanted, too. The trickiest part, though, is
> blaming deletions, because git-blame only tracks the origin of content,
> not the origin of a change.
Hmph, this is a comment without looking at what difflame does
internally, so you can ignore me if I am misunderstood what problem
you are pointing out, but I am not sure how "tracks the origin of
content" could be a problem.
If output from "git show" says this:
--- a/file
+++ b/file
@@ -1,5 +1,6 @@
a
b
-c
+C
+D
d
e
in order to annotate lines 'a', 'b', 'd', and 'e' for their origin,
you would run 'blame' on the commit the above output was taken from
(or its parent---they are in the context so either would be OK).
You know where 'C' and 'D' came from already. It's the commit you
are feeding "git show".
In order to run blame to find where 'c' came from, you need to start
at the _parent_ of the commit the above output came from, and the
hunk header shows which line range to find the final 'c'.
So...
^ permalink raw reply
* Re: [RFC] Proof of concept: Support multiple authors
From: Junio C Hamano @ 2017-01-30 20:48 UTC (permalink / raw)
To: Christian Couder; +Cc: Cornelius Schumacher, git, Josh Triplett
In-Reply-To: <CAP8UFD3=vaFupEDay-5vrMBwK_YJezysUUvySxnUUZxuW7m_WQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> I am just wondering if you have read and taken into account the
> previous threads on this mailing list about the same subject, like for
> example this one:
>
> https://public-inbox.org/git/CAOvwQ4i_HL7XGnxZrVu3oSnsbnTyxbg8Vh6vzi4c1isSrrexYQ@mail.gmail.com/
Thanks for a starting-point link.
In that discussion, another discussion in the debian BTS is
mentioned,
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880
which in turn has links to other, even earlier, discussions, but
they are all gmane links so it would be harder for those who do not
use its NNTP interface (which still works). Here are their modern
counterparts ;-)
https://public-inbox.org/git/?q=gmane:83880
https://public-inbox.org/git/?q=gmane:146223
https://public-inbox.org/git/?q=gmane:146886
The older discussions already cited the cost to update both in-tree
and out-of-tree tools not to barf when they see such a commit object
and one of the reason why we would not want to do this, and Ted Ts'o
gave us another excellent reason why not to do multiple author
header lines in a commit object header, i.e. "How often is that all
of the authors are completely equal?"
Another thing that I didn't see brought up was that it is not enough
to ensure all existing tools are updated not to barf on a commit
with multiple "author" line. You need to define what it means to
have multiple authors and how they are treated by tools in a
consistent way. Think "shortlog", for example. The tool may be
able to be tweaked not to barf, and you may be able to add a random
definition of which of the multiple authors to group a single commit
under (the "random definition" could be "show that same commit
multiple times, once for each author" or it could be "concatenate
the authors to create a single header to list co-authored commits
under, as if they were a single person", or it could be something
equally bogus), but I do not think any single solution satisfies
people's needs, and my gut feeling is that it is not worth to add
tons of options and explain them to the end-users to support these
random things that happen when there are multiple authors.
Incidentally, there recently was a discussion on extending
request-pull by adding a summary of commits by reviewers and
testers.
https://public-inbox.org/git/20170115183051.3565-1-wsa@the-dreams.de/
I would imagine, if it is to be implemented, it would boil down to
teaching shortlog that the "author" line in the commit object header
is not the only thing that matters, and that it should optionally
take notice of lines in the trailer block of the log message (e.g.
perhaps "Co-Authored-by: " trailer suggested by $gmane/146223 above
would help there).
My advice to those who want to record credit to multiple authors is
to treat the commit author line as recording the primary contact
when there is a question on the commit and nothing else. Anything
with richer semantics is better done in the trailer by enriching the
support of trailer lines with interpret-trailers framework.
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Johannes Schindelin @ 2017-01-30 20:48 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <9bcae427-d654-a671-4368-030150168102@web.de>
[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]
Hi René,
On Mon, 30 Jan 2017, René Scharfe wrote:
> Am 30.01.2017 um 16:39 schrieb Johannes Schindelin:
>
> > On Sat, 28 Jan 2017, René Scharfe wrote:
> >
> > > Add a macro for exchanging the values of variables. It allows users
> > > to avoid repetition and takes care of the temporary variable for
> > > them. It also makes sure that the storage sizes of its two
> > > parameters are the same. Its memcpy(1) calls are optimized away by
> > > current compilers.
> >
> > How certain are we that "current compilers" optimize that away? And
> > about which "current compilers" are we talking? GCC? GCC 6? Clang?
> > Clang 3? Clang 3.8.*? Visual C 2005+?
>
> GCC 4.4.7 and clang 3.2 on x86-64 compile the macro to the same object
> code as a manual swap ; see https://godbolt.org/g/F4b9M9. Both were
> released 2012.
Good. Thank you.
> That website doesn't offer Visual C(++), but since you added the
> original macro in e5a94313c0 ("Teach git-apply about '-R'", 2006) I
> guess we have that part covered. ;)
Back then, I was not able to build Git using Visual C *at all*. It
required a Herculean effort to make that happen, and it did not happen
before the Git for Windows project was started in 2007.
So I tried to verify that Visual C optimizes this well, and oh my deity,
this was not easy. In Debug mode, it does not optimize, i.e. the memcpy()
will be called, even for simple 32-bit integers. In Release mode, Visual
Studio's defaults turn on "whole-program optimization" which means that
the only swapping that is going on in the end is that the meaning of two
registers is swapped, i.e. the SWAP() macro is expanded to... no assembler
code at all.
After trying this and that and something else, I finally ended up with the
file-scope optimized SWAP() resulting in this assembly code:
00007FF791311000 mov eax,dword ptr [rcx]
00007FF791311002 mov r8d,dword ptr [rdx]
00007FF791311005 mov dword ptr [rcx],r8d
00007FF791311008 mov dword ptr [rdx],eax
However, recent events (including some discussions on this mailing list
which had unfortunate consequences) made me trust my instincts more. And
my instincts tell me that it would be unwise to replace code that swaps
primitive types in the straight-forward way by code that relies on
advanced compiler optimization to generate efficient code, otherwise
producing very suboptimal code.
The commit you quoted embarrasses me, and I have no excuse for it. I would
love to see that myswap() ugliness fixed by replacing it with a construct
that is simpler, and generates good code even without any smart compiler.
Ciao,
Dscho
^ permalink raw reply
* [PATCH 1/4] git-prompt.sh: add submodule indicator
From: Benjamin Fuchs @ 2017-01-30 20:44 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sbeller, sandals, ville.skytta, Benjamin Fuchs
In-Reply-To: <1485809065-11978-1-git-send-email-email@benjaminfuchs.de>
I expirienced that working with submodules can be confusing. This indicator
will make you notice very easy when you switch into a submodule.
The new prompt will look like this: (sub:master)
Adding a new optional env variable for the new feature.
Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
---
contrib/completion/git-prompt.sh | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 97eacd7..4c82e7f 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -93,6 +93,10 @@
# directory is set up to be ignored by git, then set
# GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
# repository level by setting bash.hideIfPwdIgnored to "false".
+#
+# If you would like __git_ps1 to indicate that you are in a submodule,
+# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
+# the branch name.
# check whether printf supports -v
__git_printf_supports_v=
@@ -284,6 +288,32 @@ __git_eread ()
test -r "$f" && read "$@" <"$f"
}
+# __git_is_submodule
+# Based on:
+# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
+__git_is_submodule ()
+{
+ local git_dir parent_git module_name path
+ # Find the root of this git repo, then check if its parent dir is also a repo
+ git_dir="$(git rev-parse --show-toplevel)"
+ module_name="$(basename "$git_dir")"
+ parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
+ if [[ -n $parent_git ]]; then
+ # List all the submodule paths for the parent repo
+ while read path
+ do
+ if [[ "$path" != "$module_name" ]]; then continue; fi
+ if [[ -d "$git_dir/../$path" ]]; then return 0; fi
+ done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null)
+ fi
+ return 1
+}
+
+__git_ps1_submodule ()
+{
+ __git_is_submodule && printf "sub:"
+}
+
# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
# when called from PS1 using command substitution
# in this mode it prints text to add to bash PS1 prompt (includes branch name)
@@ -513,8 +543,13 @@ __git_ps1 ()
b="\${__git_ps1_branch_name}"
fi
+ local sub=""
+ if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
+ sub="$(__git_ps1_submodule)"
+ fi
+
local f="$w$i$s$u"
- local gitstring="$c$b${f:+$z$f}$r$p"
+ local gitstring="$c$sub$b${f:+$z$f}$r$p"
if [ $pcmode = yes ]; then
if [ "${__git_printf_supports_v-}" != yes ]; then
--
2.7.4
^ permalink raw reply related
* [PATCH 2/4] git-prompt.sh: rework of submodule indicator
From: Benjamin Fuchs @ 2017-01-30 20:44 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sbeller, sandals, ville.skytta, Benjamin Fuchs
In-Reply-To: <1485809065-11978-1-git-send-email-email@benjaminfuchs.de>
Rework of the first patch. The prompt now will look like this:
(+name:master). I tried to considere all suggestions.
Tests still missing.
Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
---
contrib/completion/git-prompt.sh | 49 ++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 4c82e7f..c44b9a2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -95,8 +95,10 @@
# repository level by setting bash.hideIfPwdIgnored to "false".
#
# If you would like __git_ps1 to indicate that you are in a submodule,
-# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
-# the branch name.
+# set GIT_PS1_SHOWSUBMODULE to a nonempty value. In this case the name
+# of the submodule will be prepended to the branch name (e.g. module:master).
+# The name will be prepended by "+" if the currently checked out submodule
+# commit does not match the SHA-1 found in the index of the containing repository.
# check whether printf supports -v
__git_printf_supports_v=
@@ -288,30 +290,27 @@ __git_eread ()
test -r "$f" && read "$@" <"$f"
}
-# __git_is_submodule
-# Based on:
-# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
-__git_is_submodule ()
-{
- local git_dir parent_git module_name path
- # Find the root of this git repo, then check if its parent dir is also a repo
- git_dir="$(git rev-parse --show-toplevel)"
- module_name="$(basename "$git_dir")"
- parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
- if [[ -n $parent_git ]]; then
- # List all the submodule paths for the parent repo
- while read path
- do
- if [[ "$path" != "$module_name" ]]; then continue; fi
- if [[ -d "$git_dir/../$path" ]]; then return 0; fi
- done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null)
- fi
- return 1
-}
-
+# __git_ps1_submodule
+#
+# $1 - git_dir path
+#
+# Returns the name of the submodule if we are currently inside one. The name
+# will be prepended by "+" if the currently checked out submodule commit does
+# not match the SHA-1 found in the index of the containing repository.
+# NOTE: git_dir looks like in a ...
+# - submodule: "GIT_PARENT/.git/modules/PATH_TO_SUBMODULE"
+# - parent: "GIT_PARENT/.git" (exception "." in .git)
__git_ps1_submodule ()
{
- __git_is_submodule && printf "sub:"
+ local git_dir="$1"
+ local submodule_name="$(basename "$git_dir")"
+ if [ "$submodule_name" != ".git" ] && [ "$submodule_name" != "." ]; then
+ local parent_top="${git_dir%.git*}"
+ local submodule_top="${git_dir#*modules}"
+ local status=""
+ git diff -C "$parent_top" --no-ext-diff --ignore-submodules=dirty --quiet -- "$submodule_top" 2>/dev/null || status="+"
+ printf "$status$submodule_name:"
+ fi
}
# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
@@ -545,7 +544,7 @@ __git_ps1 ()
local sub=""
if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
- sub="$(__git_ps1_submodule)"
+ sub="$(__git_ps1_submodule $g)"
fi
local f="$w$i$s$u"
--
2.7.4
^ permalink raw reply related
* [PATCH 4/4] git-prompt.sh: add tests for submodule indicator
From: Benjamin Fuchs @ 2017-01-30 20:44 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sbeller, sandals, ville.skytta, Benjamin Fuchs
In-Reply-To: <1485809065-11978-1-git-send-email-email@benjaminfuchs.de>
Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
---
t/t9903-bash-prompt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 97c9b32..4dce366 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -37,6 +37,11 @@ test_expect_success 'setup for prompt tests' '
git commit -m "yet another b2" file &&
mkdir ignored_dir &&
echo "ignored_dir/" >>.gitignore &&
+ git checkout -b submodule &&
+ git submodule add ./. sub &&
+ git -C sub checkout master &&
+ git add sub &&
+ git commit -m submodule &&
git checkout master
'
@@ -755,4 +760,42 @@ test_expect_success 'prompt - hide if pwd ignored - inside gitdir (stderr)' '
test_cmp expected "$actual"
'
+test_expect_success 'prompt - submodule indicator' '
+ printf " (sub:master)" >expected &&
+ git checkout submodule &&
+ test_when_finished "git checkout master" &&
+ (
+ cd sub &&
+ GIT_PS1_SHOWSUBMODULE=1 &&
+ __git_ps1 >"$actual"
+ ) &&
+ test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - submodule indicator - verify false' '
+ printf " (master)" >expected &&
+ git checkout submodule &&
+ test_when_finished "git checkout master" &&
+ (
+ cd sub &&
+ GIT_PS1_SHOWSUBMODULE= &&
+ __git_ps1 >"$actual"
+ ) &&
+ test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - submodule indicator - dirty status indicator' '
+ printf " (+sub:b1)" >expected &&
+ git checkout submodule &&
+ git -C sub checkout b1 &&
+ test_when_finished "git checkout master" &&
+ (
+ cd sub &&
+ GIT_PS1_SHOWSUBMODULE=1 &&
+ __git_ps1 >"$actual"
+ ) &&
+ test_cmp expected "$actual"
+'
+
+
test_done
--
2.7.4
^ permalink raw reply related
* [PATCH 0/4] git-prompt.sh: Full patch for submodule indicator
From: Benjamin Fuchs @ 2017-01-30 20:44 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sbeller, sandals, ville.skytta, Benjamin Fuchs
Hi everyone,
since I didn't get a response I decided to sent my patch again. Maybe it was because
I to sent my consecutive commits the wrong way, so a new try.
First thanks again Steffen and Gábor for your feedback.
Based on the first feedback I rework the indicator and it is now way cheaper then the
first version and has a 'dirty' indicator now.
Tests exist also now.
Looking forward to more feedback!
Greetings,
Benjamin
Benjamin Fuchs (4):
git-prompt.sh: add submodule indicator
git-prompt.sh: rework of submodule indicator
git-prompt.sh: fix for submodule 'dirty' indicator
git-prompt.sh: add tests for submodule indicator
contrib/completion/git-prompt.sh | 36 ++++++++++++++++++++++++++++++++-
t/t9903-bash-prompt.sh | 43 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+), 1 deletion(-)
--
2.7.4
^ 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