* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Junio C Hamano @ 2017-01-31 21:09 UTC (permalink / raw)
To: Christian Couder
Cc: Duy Nguyen, git, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <xmqqy3xrqbkr.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Yes, but you need to realize that "it is better not to bother users
> with a report of failure to touch in read-only repository" and "we
> ignore all failures".
Sorry about an unfinished sentence here. "need to realize that
... and ... are different things."
> ... It is very similar
> to a situation where you ... run "status".
> The command first runs the equivalent of "update-index --refresh"
> only in-core, and it attempts to write the updated index because
> (1) it paid the cost of doing the refreshing already, and (2) if it
> can write into the index, it will help future operations in the
> repository. But it does not report a failure to write the index
> exactly because it is merely doing an opportunistic write.
>
> And in the "we read from the split index, and we attempt to touch
> the underlying shared one to update its timestamp" case, it is OK
> not to report if we failed to touch.
> ...
> ... On the
> other hand, if you added new information, i.e. wrote the split index
> based on it, it is a good indication that the <split,shared> index
> pair has information that is more valuable. We must warn in that
> case.
This reminds us of a third case. What should happen if we are doing
the "opportunistic writing of the index" in "git status", managed to
write the split index, but failed to touch the shared one?
In the ideal world, I think we should do the following sequence:
- "status" tries to write cache to the file.
- we try to write and on any error, we return error to the caller,
who is already prepared to ignore it and stay silent.
- as the first step of writing the index, we first try to touch
the shared one. If it fails, we return an error here without
writing the split one out.
- then we try to write the split one out. If this fails, we
also return an error.
- otherwise, both touching of the shared one and writing of the
split one are successful.
- "status" finishes the opportunistic refreshing of the index, by
either ignoring an error silently (if either touching of shared
one or writing of split one fails) or writing the refreshed index
out successfully.
It is OK to swap the order of touching the shared one and writing of
the split one in the above sequence, as long as an error in either
step signals a failure to the opportunistic caller.
I do not offhand know if the split-index code is structured in such
a way to allow the above sequence easily, or it needs refactoring.
If such a restructuring is required, it might not be within the
scope of the series and I am OK if you just left the NEEDSWORK
comment that describes the above (i.e. what we should be doing) as
an in-code comment so that we can pick it up later. The whole point
of the step 14/21 on the other hand is to make sure that a shared
index that is still in active use will not go stale, and from that
point of view, such a "punting" may not be a good idea---it
deliberately finishes the series knowing that it does not adequately
do what it promises to do.
So, ... I dunno.
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Jeff King @ 2017-01-31 21:35 UTC (permalink / raw)
To: René Scharfe
Cc: Brandon Williams, Johannes Schindelin, Johannes Sixt, Git List,
Junio C Hamano
In-Reply-To: <8e94756a-c3a5-9b81-268d-d0f36876f710@web.de>
On Tue, Jan 31, 2017 at 10:03:01PM +0100, René Scharfe wrote:
> > 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.
>
> SWAP(a++, ...) is caught by the compiler, SWAP(*a++, ...) works fine.
> Technically that should be enough. :) A comment wouldn't hurt, of course.
One funny thing is that your macro takes address-of itself, behind the
scenes. I wonder if it would be more natural for it to take
pointers-to-objects, making it look more like a real function (i.e.,
SWAP(&a, &b) instead of SWAP(a, b)". And then these funny corner cases
become quite obvious in the caller, because the caller is the one who
has to type "&".
-Peff
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-01-31 21:03 UTC (permalink / raw)
To: Brandon Williams
Cc: Johannes Schindelin, Johannes Sixt, Git List, Junio C Hamano
In-Reply-To: <20170130222157.GC35626@google.com>
Am 30.01.2017 um 23:21 schrieb Brandon Williams:
> 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.
SWAP(a++, ...) is caught by the compiler, SWAP(*a++, ...) works fine.
Technically that should be enough. :) A comment wouldn't hurt, of course.
René
^ permalink raw reply
* Re: [PATCH 3/5] use SWAP macro
From: René Scharfe @ 2017-01-31 21:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Johannes Schindelin
In-Reply-To: <xmqqr33ktch9.fsf@gitster.mtv.corp.google.com>
Am 30.01.2017 um 23:22 schrieb Junio C Hamano:
> 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.
Well, a human wrote "tmp = tree2" -- sometimes one likes to count down
instead of up, I guess.
We can make Coccinelle order the parameters alphabetically, but I don't
know how to do so without duplicating most of the semantic patch. And
thats would result in e.g. SWAP(new_tree, old_tree), which might be odd
as well.
I'd just leave them in whatever order they were, but that's probably
because I'm lazy.
René
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-01-31 21:02 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701311305030.3469@virtualbox>
Am 31.01.2017 um 13:13 schrieb Johannes Schindelin:
> Hi René,
>
> On Mon, 30 Jan 2017, René Scharfe wrote:
>
>> Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:
>>
>>> 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.
>
> Exactly. And you know what? I would be very okay with that type parameter.
>
> Coccinelle [*1*] should be able to cope with that, too, mehtinks.
Yes, a semantic patch can turn the type of the temporary variable into a
macro parameter. Programmers would have to type the type, though,
making the macro only half as good.
> It would be trivially "optimized" out of the box, even when compiling with
> Tiny C or in debug mode.
Such a compiler is already slowed down by memset(3) calls for
initializing objects and lack of other optimizations. I doubt a few
more memcpy(3) calls would make that much of a difference.
NB: git as compiled with TCC fails several tests, alas. Builds wickedly
fast, though.
> And it would even allow things like this:
>
> #define SIMPLE_SWAP(T, a, b) do { T tmp_ = a; a = b; b = tmp_; } while (0)
> ...
> uint32_t large;
> char nybble;
>
> ...
>
> if (!(large & ~0xf)) {
> SIMPLE_SWAP(char, nybble, large);
> ...
> }
>
> i.e. mixing types, when possible.
>
> And while I do not necessarily expect that we need anything like this
> anytime soon, merely the fact that it allows for this flexibility, while
> being very readable at the same time, would make it a pretty good design
> in my book.
Such a skinny macro which only hides repetition is kind of attractive
due to its simplicity; I can't say the same about the mixed type example
above, though.
The fat version isn't that bad either even without inlining, includes a
few safety checks and doesn't require us to tell the compiler something
it already knows very well. I'd rather let the machine do the work.
René
^ permalink raw reply
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Cornelius Weig @ 2017-01-31 20:28 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: git
In-Reply-To: <xmqqbmunrwbf.fsf@gitster.mtv.corp.google.com>
On 01/31/2017 06:08 PM, Junio C Hamano wrote:
> I think it is probably a good idea to document the behaviour
> (i.e. "--no-create" single-shot from the command line is ignored).
> I am not sure we should error out, though, in order to "disallow"
> it---a documented silent no-op may be sufficient.
Yes, maybe abort on seeing "--no-create-reflog" is a too drastic
measure. I presume that the best place to have the documentation would
be to print a warning when seeing the ignored argument?
Or did you just have man pages and code comment in mind?
Cheers,
Cornelius
^ permalink raw reply
* Re: vger not relaying some of Junio's messages today?
From: Junio C Hamano @ 2017-01-31 20:27 UTC (permalink / raw)
To: Jeff King; +Cc: Eric Wong, git
In-Reply-To: <20170127040139.wz7xmizccjigz5ui@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Jan 27, 2017 at 03:57:53AM +0000, Eric Wong wrote:
>
>> I noticed both of these are are missing from my archives
>> (which rejects messages unless they come from vger):
>>
>> <xmqq1svp7lcs.fsf@gitster.mtv.corp.google.com>
>> <xmqqefzp1d1x.fsf@gitster.mtv.corp.google.com>
>
> I don't have them either, so presumably vger ate them (I usually delete
> my cc copies after reading and keep the list ones, and I now have
> neither).
>
>> Not sure if there's something up with vger or Junio's setup because
>> other messages (even from Junio) are getting through fine.
>
> Sporadic failures, especially on a single topic, imply that something in
> the content may triggered the taboo filter (though often that takes out
> replies, too, which this doesn't seem to have done).
I think I figured this out yesterday.
It was a dot somewhere in the name of a recipient on To:/Cc: line
that is NOT double-quoted. For this thread, brian's human-readable
name "brian m. carlson" was double-quoted in the original I
responded to and it also was double-quoted in my response.
But there was another recipient with a dot after the middle initial,
which was NOT double-quoted in the original.
My response seems to have been eaten because that name was not
double-quoted. I saw that happen yesterday in another thread with
the same recipient, and resent with the only change to double-quote
that name and it went through.
^ permalink raw reply
* Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor
From: Junio C Hamano @ 2017-01-31 20:20 UTC (permalink / raw)
To: brian m. carlson
Cc: Eric Wong, Jeff King, git, Johannes Schindelin,
Øyvind A. Holm
In-Reply-To: <20170127004050.23jrq5iqwfxcwmik@genre.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> On Thu, Jan 26, 2017 at 07:18:41PM +0000, Eric Wong wrote:
>> > Eric Wong <e@80x24.org> writes:
>> Junio C Hamano <gitster@pobox.com> wrote:
>> > + "<citerefentry>\n"
>> > + "<refentrytitle>#{target}</refentrytitle>"
>> > + "<manvolnum>#{attrs[1]}</manvolnum>\n"
>> > + "</citerefentry>\n"
>> > end
>>
>> You need the '\' at the end of those strings, it's not like C
>> since Ruby doesn't require semi-colons to terminate lines.
>> In other words, that should be:
>>
>> "<citerefentry>\n" \
>> "<refentrytitle>#{target}</refentrytitle>" \
>> "<manvolnum>#{attrs[1]}</manvolnum>\n" \
>> "</citerefentry>\n"
>>
>
> This change is fine with me.
OK, I just squashed the final one in. Will merge to 'next' shortly.
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 11f76506b6..b21e5808b1 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -179,7 +179,8 @@ ASCIIDOC = asciidoctor
ASCIIDOC_CONF =
ASCIIDOC_HTML = xhtml5
ASCIIDOC_DOCBOOK = docbook45
-ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions -alitdd='&\#x2d;&\#x2d;'
+ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions
+ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
DBLATEX_COMMON =
endif
diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
index 09f7088eea..ec83b4959e 100644
--- a/Documentation/asciidoctor-extensions.rb
+++ b/Documentation/asciidoctor-extensions.rb
@@ -13,10 +13,10 @@ module Git
prefix = parent.document.attr('git-relative-html-prefix')
%(<a href="#{prefix}#{target}.html">#{target}(#{attrs[1]})</a>\n)
elsif parent.document.basebackend? 'docbook'
- %(<citerefentry>
-<refentrytitle>#{target}</refentrytitle><manvolnum>#{attrs[1]}</manvolnum>
-</citerefentry>
-)
+ "<citerefentry>\n" \
+ "<refentrytitle>#{target}</refentrytitle>" \
+ "<manvolnum>#{attrs[1]}</manvolnum>\n" \
+ "</citerefentry>\n"
end
end
end
^ permalink raw reply related
* Re: [PATCH] blame: draft of line format
From: Jeff King @ 2017-01-31 19:41 UTC (permalink / raw)
To: Edmundo Carmona Antoranz; +Cc: git
In-Reply-To: <20170131022830.8538-1-eantoranz@gmail.com>
On Mon, Jan 30, 2017 at 08:28:30PM -0600, Edmundo Carmona Antoranz wrote:
> +static void pretty_info(char* revid, struct blame_entry *ent, struct strbuf *rev_buffer)
> +{
> + struct pretty_print_context ctx = {0};
> + struct rev_info rev;
> +
> + struct strbuf format = STRBUF_INIT;
> + strbuf_addstr(&format, format_line);
> + ctx.fmt = CMIT_FMT_USERFORMAT;
> + get_commit_format(format.buf, &rev);
> + pretty_print_commit(&ctx, ent->suspect->commit, rev_buffer);
> + strbuf_release(&format);
> +}
I think this may be less awkward if you use format_commit_message() as
the entry point. Then you do not need a rev_info struct at all, it
touches fewer global variables, etc.
I don't know if that would cause the other difficulties you mentioned,
though.
-Peff
^ permalink raw reply
* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Junio C Hamano @ 2017-01-31 19:22 UTC (permalink / raw)
To: Christian Couder
Cc: Duy Nguyen, git, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD3iWg5g-h209VDyg1U03Jmma8nTONyCYB-kN7GwspkL8Q@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
>> You are listing only the irrelevant cases. The shared one may be
>> used immediately, and the user can keep using it for a while without
>> "touching".
>
> Now you are talking about a case where the shared index file can be
> used immediately and the user can keep using it.
> This is a different case than the case I was talking about (which is
> the case when the shared index file does not exist anymore).
Yes, as I said, you are listing irrelevant and uninteresting one.
If the shared index is already gone, reporting failure to touch it
is of no use---an attempt to read and use the split index that
depends on the shared index that is gone will fail anyway.
> In a previous email you wrote that if the file system is read only, it
> is a bad thing if we warn.
Yes, but you need to realize that "it is better not to bother users
with a report of failure to touch in read-only repository" and "we
ignore all failures".
IIUC, you attempt to touch the shared index even when you are
only reading the index, because you want to mark the fact that the
shared index is still being depended upon. And I tend to agree that
it is OK not to report a failure for that case. It is very similar
to a situation where you are asked to peek into your coworker's
repository, which you do not have write access to, and run "status".
The command first runs the equivalent of "update-index --refresh"
only in-core, and it attempts to write the updated index because
(1) it paid the cost of doing the refreshing already, and (2) if it
can write into the index, it will help future operations in the
repository. But it does not report a failure to write the index
exactly because it is merely doing an opportunistic write.
And in the "we read from the split index, and we attempt to touch
the underlying shared one to update its timestamp" case, it is OK
not to report if we failed to touch.
But you also attempt to touch the shared index when you are actually
writing out a new split index file based on it, no? The "you
created a ticking bomb" situation is where you fail to touch the
shared index for whatever reason, even when you managed to write the
new split index file.
We agreed that read-only operation should not nag, so it won't
trigger when you are peeking somebody else's repository to help him.
As I said, it is an uninteresting and irrelevant case---when your
read-only peeking did not add new information to be preserved, it is
less severe problem that you fail to reset the expiration. On the
other hand, if you added new information, i.e. wrote the split index
based on it, it is a good indication that the <split,shared> index
pair has information that is more valuable. We must warn in that
case.
> Now if you want to talk about the case when the shared index file has
> not been removed, but just chowned to "root", then I wonder how is it
> different from the case when the file system is read only.
The difference is that your code has enough information to notice
the case where you know your touch failed, you know that you wanted
to write (and the write succeeded), and yet you do NOT know why your
touch failed. That is the ticking bomb we need to warn the user
about.
^ permalink raw reply
* [PATCH] .mailmap: update Gábor Szeder's email address
From: SZEDER Gábor @ 2017-01-31 18:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, SZEDER Gábor
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)
diff --git a/.mailmap b/.mailmap
index 9c87a3840..ab59b2fac 100644
--- a/.mailmap
+++ b/.mailmap
@@ -225,6 +225,7 @@ Steven Walter <stevenrwalter@gmail.com> <swalter@lexmark.com>
Steven Walter <stevenrwalter@gmail.com> <swalter@lpdev.prtdev.lexmark.com>
Sven Verdoolaege <skimo@kotnet.org> <Sven.Verdoolaege@cs.kuleuven.ac.be>
Sven Verdoolaege <skimo@kotnet.org> <skimo@liacs.nl>
+SZEDER Gábor <szeder.dev@gmail.com> <szeder@ira.uka.de>
Tay Ray Chuan <rctay89@gmail.com>
Ted Percival <ted@midg3t.net> <ted.percival@quest.com>
Theodore Ts'o <tytso@mit.edu>
--
2.11.0.555.g967c1bcb3
^ permalink raw reply related
* Re: [PATCH 4/4] git-prompt.sh: add tests for submodule indicator
From: SZEDER Gábor @ 2017-01-31 18:32 UTC (permalink / raw)
To: Benjamin Fuchs; +Cc: git, Stefan Beller, brian m. carlson, ville.skytta
In-Reply-To: <1485809065-11978-5-git-send-email-email@benjaminfuchs.de>
On Mon, Jan 30, 2017 at 9:44 PM, Benjamin Fuchs <email@benjaminfuchs.de> wrote:
> 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' '
I was puzzled by the "verify false" here. You mean "disabled", right?
> + 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
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Jeff King @ 2017-01-31 18:21 UTC (permalink / raw)
To: Cornelius Weig; +Cc: Junio C Hamano, git
In-Reply-To: <1e341485-6fb6-243a-0b27-4035789a6f2a@tngtech.com>
On Tue, Jan 31, 2017 at 03:00:33PM +0100, Cornelius Weig wrote:
> Concerning branches, I fully agree. For git-branch, the
> "--no-create-reflog" option does not make sense at all and should
> produce an error.
>
> On the other hand, for tags it may make sense to override
> logAllRefUpdates=always. As tag updates come exclusively from
> force-creating the same tag on another revision, a reflog will actually
> not be created by accident.
Hmm. I think you could also see tag creation and update via "git fetch",
though only with explicit refspecs, I think, not tag-following.
So I think ultimately you'd need to use "git -c logallrefupdates=false"
if you want to override reflog options for all commands. A saner
interface would probably be put teaching the ref code to respect a
configured list of exceptions ("I do want reflogs for refs/tags/, but
not for refs/foo/"). But I don't think it's sensible for anybody to go
to the work of doing that, given that I haven't heard a single useful
reason for --no-create-reflog in the first place.
Personally, I'd be fine with leaving it in its current state as a known
bug that somebody may fix later, if they actually care. But if it is not
too hard to fix while we are all thinking about it, we can do that.
-Peff
^ permalink raw reply
* Re: [PATCH 2/4] git-prompt.sh: rework of submodule indicator
From: SZEDER Gábor @ 2017-01-31 18:06 UTC (permalink / raw)
To: Benjamin Fuchs; +Cc: SZEDER Gábor, sbeller, sandals, ville.skytta, git
In-Reply-To: <1485809065-11978-3-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="+"
This 'git diff' has to read the index of the parent repository, right?
That can be potentially very expensive, if the parent repository, and
thus its index, is big.
You might want to provide finer granularity controls and separate the
"$PWD is in a submodule" indicator from the "submodule commit doesn't
match" indicator. Even if someone is in general interested in the
former, he might have some huge repositories where he would prefer to
disable the latter, because executing 'git diff' would make the prompt
lag.
I'm not sure we need brand new control knobs, though. Perhaps we can
get away by checking the env and config variables used for the dirty
index and worktree status indicator, after all they, too, are about
whether to run 'git diff' for the prompt in a repository or not.
> + 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)"
In Bash, and in shells in general, the visibility of variables works
differently than in "regular" programming languages:
- Any variable existing in a caller, even the ones declared as
'local' in the caller, are visible in all callees.
This means you don't have to pass $g as parameter to
__git_ps1_submodule(), because you can access it inside that
function directly. This has the benefit that there is one less
place where you can forget to quote a path variable :)
- If the callee modifies any variable that isn't declared as local
in the callee, then the caller will see the modified value of that
variable, unless the callee was invoked in a subshell.
Here your sole purpose is to set $sub to something like "+sub:",
which is determined in __git_ps1_submodule(). You don't need the
command substitution for that, you can set $sub directly in
__git_ps1_submodule(), sparing the overhead of fork()ing a
subshell. That's important for the sake of git users on Windows.
> fi
>
> local f="$w$i$s$u"
--
2.7.4
^ permalink raw reply
* Feature request: flagging “volatile” branches for integration/development
From: Herbert, Marc @ 2017-01-31 17:12 UTC (permalink / raw)
To: git; +Cc: josh
(Thanks to Josh Triplett[*] for contributing to this message)
Hi,
We often work with development/integration branches that regularly
rebase, in addition to stable branches that do not. Git is used to share
two different types of branches:
1. Pull requests and merged code with final SHA1s
2. Work in progress with volatile SHA1s.
We’d like to have a consistent way to distinguish these two types by
advertising a branch as “volatile”. Such a branch supports shared
development on work-in-progress code (not yet ready to merge, or still
being emailed as PATCHv{2,3,4,...}), or an integration/testing branch
for a combination of such development branches. Branch naming
conventions could help a bit here, but a large and varied set of naming
conventions already exist, none of which provide machine-readable
information that git itself can rely on. So the only thing available is
tribal knowledge or out-of-band documentation at best, e.g.:
https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-intel.html
Such a “volatile” flag would instantly warn that code is not ready to
re-use:
https://www.chromium.org/chromium-os/chromiumos-design-docs/upstream-first
Another common issue with volatile branches is their commits being
misunderstood as immutable and then their transient, top SHA1
being reported as bogus and unusable version control information in bugs
(on a non-volatile branch, a single SHA1 captures the entire and exact
snapshot and history).
Humans would be the initial consumers of this flag but I can imagine git
itself also using it. For instance, cherry-pick could have a “smart”
setting for -x, that doesn’t bother recording transient commit hashes. A
merge could print an optional warning when pulling in changes from a
volatile branch, and a rebase could similarly print a warning when
rebasing on top of such a branch. A git server could be configured to
treat non-fast forward forced pushes differently depending on the
“volatility” of the target branch. A fancy user interface could
color volatile SHA1s differently to discourage copy/paste. Etc.
Maybe this has already been discussed (or implemented even), and I
couldn’t find the right search keywords; in this case please help me cut
this discussion short. We’d appreciate any feedback you might have,
either on the idea itself, or on other ways to solve the same problem.
[ ] “send patches”
[ ] use some other existing mechanism to solve this
[ ] will never work because of X and Y; don’t even bother
--
Marc
PS: please keep me in Cc:, thanks.
[*] on a related topic: https://github.com/git-series/git-series
^ permalink raw reply
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Junio C Hamano @ 2017-01-31 17:08 UTC (permalink / raw)
To: Jeff King; +Cc: cornelius.weig, git
In-Reply-To: <20170130233702.o6naszpz32juf5gt@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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.
Thanks for a reasoned argument and a reasonable justification. I
agree with all that.
I think it is probably a good idea to document the behaviour
(i.e. "--no-create" single-shot from the command line is ignored).
I am not sure we should error out, though, in order to "disallow"
it---a documented silent no-op may be sufficient.
^ permalink raw reply
* Re: [PATCH] t0001: don't let a default ACL interfere with the umask test
From: Junio C Hamano @ 2017-01-31 17:13 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1485636766.2482.3.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> The "init creates a new deep directory (umask vs. shared)" test expects
> the permissions of newly created files to be based on the umask, which
> fails if a default ACL is inherited from the working tree for git. So
> attempt to remove a default ACL if there is one. Same idea as
> 8ed0a740dd42bd0724aebed6e3b07c4ea2a2d5e8. (I guess I'm the only one who
> ever runs the test suite with a default ACL set.)
Thanks--people with such a configuration who run tests are valuable ;-)
^ permalink raw reply
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Junio C Hamano @ 2017-01-31 17:11 UTC (permalink / raw)
To: Cornelius Weig; +Cc: peff, git
In-Reply-To: <68b6ac92-459d-849d-9589-e1fa500e2572@tngtech.com>
Cornelius Weig <cornelius.weig@tngtech.com> writes:
> And again, thanks for not yelling. I overlooked that the
> "should_autocreate_reflog" return value should have been negated as
> shown below.
Heh---I AM blind. I didn't spot it even though I was staring at the
code and even tweaking it (for the constness thing).
> Should I resend this patch, or is it easier for you
> to do the change yourself?
I can squash it in, now we have and the list saw all the bits
necessary.
Thanks for working on this.
> Interdiff v2..v3:
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 81ea2ed..1e8631a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -612,8 +612,10 @@ 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);
> - if (opts->new_branch_log && should_autocreate_reflog(refname)) {
> + 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
* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Christian Couder @ 2017-01-31 14:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, git, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <xmqqsho6amm7.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 25, 2017 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Well, when we cannot freshen a loose file (with
>> freshen_loose_object()), we don't warn or die, we just write the loose
>> file. But here we cannot write the shared index file.
>
> I think that is an excellent point. Let me make sure I got you
> right. For loose object files, we may attempt to freshen and when
> it fails we stay silent and do not talk about the failure. Instead,
> we write the same file again. That will have two potential outcomes:
>
> 1. write fails and we fail the whole thing. It is very clear to
> the user that there is something wrong going on.
>
> 2. write succeeds, and because we just wrote it, we know that the
> file is fresh and is protected from gc.
>
> So the "freshen and if fails just write" is sufficient. It is
> absolutely the right thing to do for loose object files.
>
> When we are forking off a new split index file based on an old
> shared index file, we may attempt to "touch" the old shared one, but
> we cannot write the old shared one again, because other split index
> may be based on that, and we do not have enough information to
> recreate the old one [*1*]. The fallback safety is not available.
Yeah I agree. But note that I was talking about the case where the
shared index file does not exist anymore.
I was just saying that when the shared index file has been removed
behind us, it is equivalent as when
loose files have been removed behind us, except that we cannot write
the removed file to disk.
>> And this doesn't lead to a catastrophic failure right away.
>
> Exactly.
>
>> There
>> could be a catastrophic failure if the shared index file is needed
>> again later, but we are not sure that it's going to be needed later.
>> In fact it may have just been removed because it won't be needed
>> later.
>
> You are listing only the irrelevant cases. The shared one may be
> used immediately, and the user can keep using it for a while without
> "touching".
Now you are talking about a case where the shared index file can be
used immediately and the user can keep using it.
This is a different case than the case I was talking about (which is
the case when the shared index file does not exist anymore).
> Perhaps the shared one was chowned to "root" while the
> user is looking the other way, and its timestamp is now frozen to
> the time of chown. It is a ticking time-bomb that will go off when
> your expiry logic kicks in.
In the case I was talking about, the shared index file doesn't exist
anymore. So there is no time ticking bomb, because what could happen,
if we cannot freshen the shared index file, is that the shared index
file could be removed, which wouldn't make things worse as anyway it
does not exist anymore.
So the case when the shared index file doesn't exist is different than
other cases.
Now if you want to talk about the case when the shared index file has
not been removed, but just chowned to "root", then I wonder how is it
different from the case when the file system is read only.
Perhaps the admin just chowned everything to make sure that the repo
could not be changed anymore by the usual processes and users.
>> So I am not sure it's a good idea to anticipate a catastrophic failure
>> that may not happen. Perhaps we could just warn, but I am not sure it
>> will help the user. If the catastrophic failure doesn't happen because
>> the shared index file is not needed, I can't see how the warning could
>> help. And if there is a catastrophic failure, the following will be
>> displayed:
>>
>> fatal: .git/sharedindex.cbfe41352a4623d4d3e9757861fed53f3094e0ac:
>> index file open failed: No such file or directory
>
> That "fatal" is given _ONLY_ after time passes and our failure to
> "touch" the file that is still-in-use left it subject to "gc". Of
> course, when "fatal" is given, it is too late to warn about ticking
> time bomb.
>
> At the time we notice a ticking time bomb is the only sensible time
> to warn. Or better yet take a corrective action.
In a previous email you wrote that if the file system is read only, it
is a bad thing if we warn.
So I wonder why it would be a good thing to warn or try to do
something if the admin chowned everything to prevent usual processes
and users to change anything.
> As I expect (and you seem to agree) that a failure to "touch" would
> be a very rare event (like, sysadmin chowning it to "root" by
> mistake),
Yeah, I agree that a failure to "touch" would be a rare event. But the
thing is we often don't know if admins or users do things by mistake
or not.
If they rm -rf everything forcefully, for example, they might not like
it, if we start to rewrite stuff to try to "correct" things.
> I do not mind if the "corrective action" were "immediately
> unsplit the index, so that at least the current and the latest index
> contents will be written out safely to a new single unsplit index
> file".
If the admins or users chowned a shared index file precisely because
they want to save it from ever being garbage collected, I am not sure
that they would be happy if we decide to "correct" things.
For example they might not like it if we change the current mode to
unsplit index.
So in my opinion, the safest thing, if we really want to do something,
is to just warn, perhaps once, and I am ok to do that.
If we really wanted to do our best, perhaps we could warn once only in
the case when the shared index file still exists (because otherwise
there nothing to lose anymore).
> That won't help _other_ split index files that are based on
> the same "untouchable" shared index, but I do not think that is a
> problem we need to solve---if they are still in use, the code that
> use them will notice it, and otherwise they are not in use and can
> be aged away safely.
I am not sure that I understand you correctly on this. The current
code does not age away the split index files. It only ages away shared
index files.
I think aging away the split index file is a different issue, because
when we are not in split index mode, the index files are not aged
anyway.
^ permalink raw reply
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Cornelius Weig @ 2017-01-31 14:00 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20170130233702.o6naszpz32juf5gt@sigill.intra.peff.net>
On 01/31/2017 12:37 AM, Jeff King wrote:
> 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".
I was actually not referring to update-ref, for which the
--no-create-reflog option works fine. I was referring to git-tag which
also has the --create-reflog option. For git-tag, the current code does
not allow to override logAllRefUpdates=always with --no-create-reflog.
On the other hand logAllRefUpdates=false is overridden by "git tag
--create-reflog". The reason is that the file-backend does allow to
force reflog creation, but it does not allow to force reflog
non-creation. I have a patch that amends this, but it's not pretty and I
don't think it will be useful (see last paragraph).
> I hadn't thought about that. I think "git branch --no-create-reflog" has
> the same problem in the existing code.
You are right, git-branch also ignores --no-create-reflog.
> 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.
Concerning branches, I fully agree. For git-branch, the
"--no-create-reflog" option does not make sense at all and should
produce an error.
On the other hand, for tags it may make sense to override
logAllRefUpdates=always. As tag updates come exclusively from
force-creating the same tag on another revision, a reflog will actually
not be created by accident.
Nevertheless, I don't think it is very useful to have the
"--no-create-reflog" argument to any of git-branch or git-tag. It only
takes effect if a user has configured logAllRefUpdates=always, and he
probably has done that for a reason. Given that the overhead from a
reflog is minuscule, IMHO no-one will ever bother about
"--no-create-reflog".
^ permalink raw reply
* [PATCHv4] builtin/commit.c: switch to strbuf, instead of snprintf()
From: Elia Pinto @ 2017-01-31 13:45 UTC (permalink / raw)
To: git; +Cc: Elia Pinto
Switch to dynamic allocation with strbuf, so we can avoid dealing
with magic numbers in the code and reduce the cognitive burden from
the programmers. The original code is correct, but programmers no
longer have to count bytes needed for static allocation to know that.
As a side effect of this change, we also reduce the snprintf()
calls, that may silently truncate results if the programmer is not
careful.
Helped-by: René Scharfe <l.s.r@web.de>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
This is the fourth patch version.
Changes from the first version: I have split the original commit in two, as discussed here
http://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.
Changes from the second version:
- Changed the commit message to clarify the purpose of the patch (
as suggested by Junio)
Changes from the third version:
- Swith to use strbuf instead of xstrfmt (
as suggested by René Scharfe
here https://public-inbox.org/git/3165803b-6073-de97-bf33-71ad21108d6f@web.de/)
builtin/commit.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 276c4f2e7..2de5f6cc6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1525,12 +1525,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
static int run_rewrite_hook(const unsigned char *oldsha1,
const unsigned char *newsha1)
{
- /* oldsha1 SP newsha1 LF NUL */
- static char buf[2*40 + 3];
struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
int code;
- size_t n;
+ struct strbuf sb = STRBUF_INIT;
argv[0] = find_hook("post-rewrite");
if (!argv[0])
@@ -1546,11 +1544,11 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
code = start_command(&proc);
if (code)
return code;
- n = snprintf(buf, sizeof(buf), "%s %s\n",
- sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+ strbuf_addf(&sb, "%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
sigchain_push(SIGPIPE, SIG_IGN);
- write_in_full(proc.in, buf, n);
+ write_in_full(proc.in, sb.buf, sb.len);
close(proc.in);
+ strbuf_release(&sb);
sigchain_pop(SIGPIPE);
return finish_command(&proc);
}
--
2.11.0.297.g2a7e328.dirty
^ permalink raw reply related
* Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always
From: Cornelius Weig @ 2017-01-31 13:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: peff, git
In-Reply-To: <xmqq8tpstaus.fsf@gitster.mtv.corp.google.com>
Hi,
> 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.
Ooops.. thanks for not yelling at me for that :-/
> 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).
And again, thanks for not yelling. I overlooked that the
"should_autocreate_reflog" return value should have been negated as
shown below. Should I resend this patch, or is it easier for you
to do the change yourself?
Interdiff v2..v3:
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 81ea2ed..1e8631a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -612,8 +612,10 @@ 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);
- if (opts->new_branch_log && should_autocreate_reflog(refname)) {
+ 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 1/5] add SWAP macro
From: Johannes Schindelin @ 2017-01-31 12:13 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <6760493c-3684-b8bb-2c01-6621b8417246@web.de>
[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]
Hi René,
On Mon, 30 Jan 2017, René Scharfe wrote:
> Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:
>
> > 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.
Exactly. And you know what? I would be very okay with that type parameter.
Coccinelle [*1*] should be able to cope with that, too, mehtinks.
It would be trivially "optimized" out of the box, even when compiling with
Tiny C or in debug mode.
And it would even allow things like this:
#define SIMPLE_SWAP(T, a, b) do { T tmp_ = a; a = b; b = tmp_; } while (0)
...
uint32_t large;
char nybble;
...
if (!(large & ~0xf)) {
SIMPLE_SWAP(char, nybble, large);
...
}
i.e. mixing types, when possible.
And while I do not necessarily expect that we need anything like this
anytime soon, merely the fact that it allows for this flexibility, while
being very readable at the same time, would make it a pretty good design
in my book.
Ciao,
Dscho
P.S.: I am sure glad they use the French name and do not translate it to
English.
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Johannes Schindelin @ 2017-01-31 12:03 UTC (permalink / raw)
To: René Scharfe; +Cc: Johannes Sixt, Git List, Junio C Hamano
In-Reply-To: <77098ac8-1084-a5c4-a5e6-fb382e3dc3a0@web.de>
[-- Attachment #1: Type: text/plain, Size: 985 bytes --]
Hi René,
On Mon, 30 Jan 2017, 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.
Thank you for clarifying.
> > 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".
Okay, now we know what the tool says.
I would like to take a step back and state that it does not make much
sense to support SWAP(a++, b), as it is confusing at best.
Ciao,
Dscho
^ permalink raw reply
* [PATCH v5 4/5] urlmatch: include host in urlmatch ranking
From: Patrick Steinhardt @ 2017-01-31 9:01 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <cover.1485853153.git.ps@pks.im>
From: Patrick Steinhardt <patrick.steinhardt@elego.de>
In order to be able to rank positive matches by `urlmatch`, we inspect
the path length and user part to decide whether a match is better than
another match. As all other parts are matched exactly between both URLs,
this is the correct thing to do right now.
In the future, though, we want to introduce wild cards for the domain
part. When doing this, it does not make sense anymore to only compare
the path lengths. Instead, we also want to compare the domain lengths to
determine which of both URLs matches the host part more closely.
Signed-off-by: Patrick Steinhardt <patrick.steinhardt@elego.de>
---
t/t1300-repo-config.sh | 33 ++++++++++++++++++++++++++++
urlmatch.c | 59 +++++++++++++++++++++++++++++---------------------
urlmatch.h | 3 ++-
3 files changed, 69 insertions(+), 26 deletions(-)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 923bfc5a2..6c844d519 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1177,6 +1177,39 @@ test_expect_success 'urlmatch' '
test_cmp expect actual
'
+test_expect_success 'urlmatch favors more specific URLs' '
+ cat >.git/config <<-\EOF &&
+ [http "https://example.com/"]
+ cookieFile = /tmp/root.txt
+ [http "https://example.com/subdirectory"]
+ cookieFile = /tmp/subdirectory.txt
+ [http "https://user@example.com/"]
+ cookieFile = /tmp/user.txt
+ [http "https://averylonguser@example.com/"]
+ cookieFile = /tmp/averylonguser.txt
+ EOF
+
+ echo http.cookiefile /tmp/root.txt >expect &&
+ git config --get-urlmatch HTTP https://example.com >actual &&
+ test_cmp expect actual &&
+
+ echo http.cookiefile /tmp/subdirectory.txt >expect &&
+ git config --get-urlmatch HTTP https://example.com/subdirectory >actual &&
+ test_cmp expect actual &&
+
+ echo http.cookiefile /tmp/subdirectory.txt >expect &&
+ git config --get-urlmatch HTTP https://example.com/subdirectory/nested >actual &&
+ test_cmp expect actual &&
+
+ echo http.cookiefile /tmp/user.txt >expect &&
+ git config --get-urlmatch HTTP https://user@example.com/ >actual &&
+ test_cmp expect actual &&
+
+ echo http.cookiefile /tmp/subdirectory.txt >expect &&
+ git config --get-urlmatch HTTP https://averylonguser@example.com/subdirectory >actual &&
+ test_cmp expect actual
+'
+
# good section hygiene
test_expect_failure 'unsetting the last key in a section removes header' '
cat >.git/config <<-\EOF &&
diff --git a/urlmatch.c b/urlmatch.c
index e328905eb..f79887825 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -426,7 +426,7 @@ static size_t url_match_prefix(const char *url,
static int match_urls(const struct url_info *url,
const struct url_info *url_prefix,
- int *exactusermatch)
+ struct urlmatch_item *match)
{
/*
* url_prefix matches url if the scheme, host and port of url_prefix
@@ -445,8 +445,8 @@ static int match_urls(const struct url_info *url,
* contained a user name or false if url_prefix did not have a
* user name. If there is no match *exactusermatch is left untouched.
*/
- int usermatched = 0;
- int pathmatchlen;
+ char usermatched = 0;
+ size_t pathmatchlen;
if (!url || !url_prefix || !url->url || !url_prefix->url)
return 0;
@@ -483,22 +483,38 @@ static int match_urls(const struct url_info *url,
url->url + url->path_off,
url_prefix->url + url_prefix->path_off,
url_prefix->url_len - url_prefix->path_off);
+ if (!pathmatchlen)
+ return 0; /* paths do not match */
- if (pathmatchlen && exactusermatch)
- *exactusermatch = usermatched;
- return pathmatchlen;
+ if (match) {
+ match->hostmatch_len = url_prefix->host_len;
+ match->pathmatch_len = pathmatchlen;
+ match->user_matched = usermatched;
+ }
+
+ return 1;
+}
+
+static int cmp_matches(const struct urlmatch_item *a,
+ const struct urlmatch_item *b)
+{
+ if (a->hostmatch_len != b->hostmatch_len)
+ return a->hostmatch_len < b->hostmatch_len ? -1 : 1;
+ if (a->pathmatch_len != b->pathmatch_len)
+ return a->pathmatch_len < b->pathmatch_len ? -1 : 1;
+ if (a->user_matched != b->user_matched)
+ return b->user_matched ? -1 : 1;
+ return 0;
}
int urlmatch_config_entry(const char *var, const char *value, void *cb)
{
struct string_list_item *item;
struct urlmatch_config *collect = cb;
- struct urlmatch_item *matched;
+ struct urlmatch_item matched = {0};
struct url_info *url = &collect->url;
const char *key, *dot;
struct strbuf synthkey = STRBUF_INIT;
- size_t matched_len = 0;
- int user_matched = 0;
int retval;
if (!skip_prefix(var, collect->section, &key) || *(key++) != '.') {
@@ -516,9 +532,9 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb)
free(config_url);
if (!norm_url)
return 0;
- matched_len = match_urls(url, &norm_info, &user_matched);
+ retval = match_urls(url, &norm_info, &matched);
free(norm_url);
- if (!matched_len)
+ if (!retval)
return 0;
key = dot + 1;
}
@@ -528,24 +544,17 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb)
item = string_list_insert(&collect->vars, key);
if (!item->util) {
- matched = xcalloc(1, sizeof(*matched));
- item->util = matched;
+ item->util = xcalloc(1, sizeof(matched));
} else {
- matched = item->util;
- /*
- * Is our match shorter? Is our match the same
- * length, and without user while the current
- * candidate is with user? Then we cannot use it.
- */
- if (matched_len < matched->matched_len ||
- ((matched_len == matched->matched_len) &&
- (!user_matched && matched->user_matched)))
+ if (cmp_matches(&matched, item->util) <= 0)
+ /*
+ * Our match is worse than the old one,
+ * we cannot use it.
+ */
return 0;
- /* Otherwise, replace it with this one. */
}
- matched->matched_len = matched_len;
- matched->user_matched = user_matched;
+ memcpy(item->util, &matched, sizeof(matched));
strbuf_addstr(&synthkey, collect->section);
strbuf_addch(&synthkey, '.');
strbuf_addstr(&synthkey, key);
diff --git a/urlmatch.h b/urlmatch.h
index 0ea812b03..37ee5da85 100644
--- a/urlmatch.h
+++ b/urlmatch.h
@@ -34,7 +34,8 @@ struct url_info {
extern char *url_normalize(const char *, struct url_info *);
struct urlmatch_item {
- size_t matched_len;
+ size_t hostmatch_len;
+ size_t pathmatch_len;
char user_matched;
};
--
2.11.0
^ permalink raw reply related
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