* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Jeff King @ 2017-01-30 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: cornelius.weig, git
In-Reply-To: <xmqq37g0us5p.fsf@gitster.mtv.corp.google.com>
On Mon, Jan 30, 2017 at 01:58:10PM -0800, Junio C Hamano wrote:
> > 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 ;-)
I hadn't thought about that. I think "git branch --no-create-reflog" has
the same problem in the existing code.
I suspect nobody cares much in practice. Even if you say "don't create a
reflog now", if you have core.logAllRefUpdates turned on, then it's
likely that some _other_ operation will create the reflog later
accidentally (e.g., as soon as you "git checkout foo && git commit",
you'll get a reflog). I think you're fighting an uphill battle to turn
logAllRefUpdates on and then try to disable some reflogs selectively.
So I agree the current behavior is quietly broken, which is not good.
But I wonder if "--no-create-reflog" is really sane in the first place,
and whether we might be better off to simply disallow it.
-Peff
^ permalink raw reply
* Re: difflame
From: Jeff King @ 2017-01-30 23:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Edmundo Carmona Antoranz, Git List
In-Reply-To: <xmqqd1f4uug6.fsf@gitster.mtv.corp.google.com>
On Mon, Jan 30, 2017 at 01:08:41PM -0800, Junio C Hamano wrote:
> 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".
I think the point (or at least as I understand it) is that the diff is
not "git show" for a particular commit. It could be part of a much
larger diff that covers many commits.
As a non-hypothetical instance, I have a fork of git.git that has
various enhancements. I want to feed those enhancements upstream. I need
to know which commits should be cherry-picked to get those various
enhancements.
Looking at "log origin..fork" tells me too many commits, because it
includes ones which aren't useful anymore. Either because they already
went upstream, or because they were cherry-picked to the fork and their
upstream counterparts merged (or even equivalent commits made upstream
that obsoleted the features).
Looking at "git diff origin fork" tells me what the actual differences
are, but it doesn't show me which commits are responsible for them.
I can "git blame" each individual line of the diff (starting with "fork"
as the tip), but that doesn't work for lines that no longer exist (i.e.,
when the interesting change is a deletion).
> 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 perhaps that explains my comment more. "blame" is not good for
finding which commit took away a line. I've tried using "blame
--reverse", but it shows you the parent of the commit you are looking
for, which is slightly annoying. :)
"git log -S" is probably a better tool for finding that.
-Peff
^ permalink raw reply
* Re: [PATCH] merge-recursive: make "CONFLICT (rename/delete)" message show both paths
From: Junio C Hamano @ 2017-01-30 23:21 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1485636764.2482.2.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> The current message printed by "git merge-recursive" for a rename/delete
> conflict is like this:
>
> CONFLICT (rename/delete): new-path deleted in HEAD and renamed in
> other-branch. Version other-branch of new-path left in tree.
>
> To be more helpful, the message should show both paths of the rename and
> state that the deletion occurred at the old path, not the new path. So
> change the message to the following format:
>
> CONFLICT (rename/delete): old-path deleted in HEAD and renamed to
> new-path in other-branch. Version other-branch of new-path left in tree.
Sounds like a sensible goal.
> Please check that my refactoring is indeed correct! All the existing tests pass
> for me, but the existing test coverage of these conflict messages looks poor.
This unfortunately is doing a bit too many things at once from that
point of view. I need to reserve a solid quiet 20-minutes without
distraction to check it, which I am hoping to do tonight.
Thanks.
>
> merge-recursive.c | 117 ++++++++++++++++++++++-------------------
> t/t6045-merge-rename-delete.sh | 23 ++++++++
> 2 files changed, 86 insertions(+), 54 deletions(-)
> create mode 100755 t/t6045-merge-rename-delete.sh
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d327209..e8fce10 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1061,16 +1061,20 @@ static int merge_file_one(struct merge_options *o,
> }
>
> static int handle_change_delete(struct merge_options *o,
> - const char *path,
> + const char *path, const char *old_path,
> const struct object_id *o_oid, int o_mode,
> - const struct object_id *a_oid, int a_mode,
> - const struct object_id *b_oid, int b_mode,
> + const struct object_id *changed_oid,
> + int changed_mode,
> + const char *change_branch,
> + const char *delete_branch,
> const char *change, const char *change_past)
> {
> - char *renamed = NULL;
> + char *alt_path = NULL;
> + const char *update_path = path;
> int ret = 0;
> +
> if (dir_in_way(path, !o->call_depth, 0)) {
> - renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
> + update_path = alt_path = unique_path(o, path, change_branch);
> }
>
> if (o->call_depth) {
> @@ -1081,43 +1085,43 @@ static int handle_change_delete(struct merge_options *o,
> */
> ret = remove_file_from_cache(path);
> if (!ret)
> - ret = update_file(o, 0, o_oid, o_mode,
> - renamed ? renamed : path);
> - } else if (!a_oid) {
> - if (!renamed) {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> - "and %s in %s. Version %s of %s left in tree."),
> - change, path, o->branch1, change_past,
> - o->branch2, o->branch2, path);
> - ret = update_file(o, 0, b_oid, b_mode, path);
> - } else {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> - "and %s in %s. Version %s of %s left in tree at %s."),
> - change, path, o->branch1, change_past,
> - o->branch2, o->branch2, path, renamed);
> - ret = update_file(o, 0, b_oid, b_mode, renamed);
> - }
> + ret = update_file(o, 0, o_oid, o_mode, update_path);
> } else {
> - if (!renamed) {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> - "and %s in %s. Version %s of %s left in tree."),
> - change, path, o->branch2, change_past,
> - o->branch1, o->branch1, path);
> + if (!alt_path) {
> + if (!old_path) {
> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> + "and %s in %s. Version %s of %s left in tree."),
> + change, path, delete_branch, change_past,
> + change_branch, change_branch, path);
> + } else {
> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> + "and %s to %s in %s. Version %s of %s left in tree."),
> + change, old_path, delete_branch, change_past, path,
> + change_branch, change_branch, path);
> + }
> } else {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> - "and %s in %s. Version %s of %s left in tree at %s."),
> - change, path, o->branch2, change_past,
> - o->branch1, o->branch1, path, renamed);
> - ret = update_file(o, 0, a_oid, a_mode, renamed);
> + if (!old_path) {
> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> + "and %s in %s. Version %s of %s left in tree at %s."),
> + change, path, delete_branch, change_past,
> + change_branch, change_branch, path, alt_path);
> + } else {
> + output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> + "and %s to %s in %s. Version %s of %s left in tree at %s."),
> + change, old_path, delete_branch, change_past, path,
> + change_branch, change_branch, path, alt_path);
> + }
> }
> /*
> - * No need to call update_file() on path when !renamed, since
> - * that would needlessly touch path. We could call
> - * update_file_flags() with update_cache=0 and update_wd=0,
> - * but that's a no-op.
> + * No need to call update_file() on path when change_branch ==
> + * o->branch1 && !alt_path, since that would needlessly touch
> + * path. We could call update_file_flags() with update_cache=0
> + * and update_wd=0, but that's a no-op.
> */
> + if (change_branch != o->branch1 || alt_path)
> + ret = update_file(o, 0, changed_oid, changed_mode, update_path);
> }
> - free(renamed);
> + free(alt_path);
>
> return ret;
> }
> @@ -1125,28 +1129,17 @@ static int handle_change_delete(struct merge_options *o,
> static int conflict_rename_delete(struct merge_options *o,
> struct diff_filepair *pair,
> const char *rename_branch,
> - const char *other_branch)
> + const char *delete_branch)
> {
> const struct diff_filespec *orig = pair->one;
> const struct diff_filespec *dest = pair->two;
> - const struct object_id *a_oid = NULL;
> - const struct object_id *b_oid = NULL;
> - int a_mode = 0;
> - int b_mode = 0;
> -
> - if (rename_branch == o->branch1) {
> - a_oid = &dest->oid;
> - a_mode = dest->mode;
> - } else {
> - b_oid = &dest->oid;
> - b_mode = dest->mode;
> - }
>
> if (handle_change_delete(o,
> o->call_depth ? orig->path : dest->path,
> + o->call_depth ? NULL : orig->path,
> &orig->oid, orig->mode,
> - a_oid, a_mode,
> - b_oid, b_mode,
> + &dest->oid, dest->mode,
> + rename_branch, delete_branch,
> _("rename"), _("renamed")))
> return -1;
>
> @@ -1665,11 +1658,27 @@ static int handle_modify_delete(struct merge_options *o,
> struct object_id *a_oid, int a_mode,
> struct object_id *b_oid, int b_mode)
> {
> + const char *modify_branch, *delete_branch;
> + struct object_id *changed_oid;
> + int changed_mode;
> +
> + if (a_oid) {
> + modify_branch = o->branch1;
> + delete_branch = o->branch2;
> + changed_oid = a_oid;
> + changed_mode = a_mode;
> + } else {
> + modify_branch = o->branch2;
> + delete_branch = o->branch1;
> + changed_oid = b_oid;
> + changed_mode = b_mode;
> + }
> +
> return handle_change_delete(o,
> - path,
> + path, NULL,
> o_oid, o_mode,
> - a_oid, a_mode,
> - b_oid, b_mode,
> + changed_oid, changed_mode,
> + modify_branch, delete_branch,
> _("modify"), _("modified"));
> }
>
> diff --git a/t/t6045-merge-rename-delete.sh b/t/t6045-merge-rename-delete.sh
> new file mode 100755
> index 0000000..8f33244
> --- /dev/null
> +++ b/t/t6045-merge-rename-delete.sh
> @@ -0,0 +1,23 @@
> +#!/bin/sh
> +
> +test_description='Merge-recursive rename/delete conflict message'
> +. ./test-lib.sh
> +
> +test_expect_success 'rename/delete' '
> +echo foo >A &&
> +git add A &&
> +git commit -m "initial" &&
> +
> +git checkout -b rename &&
> +git mv A B &&
> +git commit -m "rename" &&
> +
> +git checkout master &&
> +git rm A &&
> +git commit -m "delete" &&
> +
> +test_must_fail git merge --strategy=recursive rename >output &&
> +test_i18ngrep "CONFLICT (rename/delete): A deleted in HEAD and renamed to B in rename. Version rename of B left in tree." output
> +'
> +
> +test_done
^ permalink raw reply
* Re: [PATCH 0/5] introduce SWAP macro
From: Junio C Hamano @ 2017-01-30 23:20 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Johannes Schindelin
In-Reply-To: <8ef4c833-45bd-6831-0683-6d01f30aa518@web.de>
René Scharfe <l.s.r@web.de> writes:
> Exchanging the value of two variables requires declaring a temporary
> variable and repeating their names. The swap macro in apply.c
> simplifies this for its callers without changing the compiled binary.
> Polish this macro and export it, then use it throughout the code to
> reduce repetition and hide the declaration of temporary variables.
All looked reasonable (modulo "swap tree2 and tree1???" ordering).
Also the object-id ones looked good.
Thanks.
^ permalink raw reply
* 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
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