* Re: [PATCH 11/12] receive-pack: treat namespace .have lines like alternates
From: Junio C Hamano @ 2017-01-25 19:51 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170124004805.nu3w47isrb4bxgi5@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Namely, de-duplicate them. We use the same set as the
> alternates, since we call them both ".have" (i.e., there is
> no value in showing one versus the other).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/receive-pack.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 8f8762e4a..c55e2f993 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -251,8 +251,9 @@ static void show_ref(const char *path, const unsigned char *sha1)
> }
>
> static int show_ref_cb(const char *path_full, const struct object_id *oid,
> - int flag, void *unused)
> + int flag, void *data)
> {
> + struct oidset *seen = data;
> const char *path = strip_namespace(path_full);
>
> if (ref_is_hidden(path, path_full))
> @@ -263,8 +264,11 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid,
> * refs, so that the client can use them to minimize data
> * transfer but will otherwise ignore them.
> */
> - if (!path)
> + if (!path) {
> + if (oidset_insert(seen, oid))
> + return 0;
> path = ".have";
> + }
This is very sensible as an optimization that does not change
semantics.
This is an unrelated tangent, but there may want to be a knob to
make the code return here without even showing, to make the
advertisement even smaller and also to stop miniscule information
leakage? If the namespaced multiple projects are totally unrelated
(i.e. "My sysadmin gave me a write access only to this single
directory, so I am using the namespace feature to host these three
projects that have nothing to do with each other"), showing objects
of other namespaces will buy us nothing and the user is better off
without this code showing these refs as ".have".
> show_ref(path, oid->hash);
> return 0;
> }
> @@ -287,7 +291,7 @@ static void write_head_info(void)
>
> for_each_alternate_ref(show_one_alternate_ref, &seen);
> oidset_clear(&seen);
> - for_each_ref(show_ref_cb, NULL);
> + for_each_ref(show_ref_cb, &seen);
> if (!sent_capabilities)
> show_ref("capabilities^{}", null_sha1);
^ permalink raw reply
* Re: [PATCH 09/12] receive-pack: use oidset to de-duplicate .have lines
From: Jeff King @ 2017-01-25 19:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqefzraqbu.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 25, 2017 at 11:32:05AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > If you have an alternate object store with a very large
> > number of refs, the peak memory usage of the sha1_array can
> > grow high, even if most of them are duplicates that end up
> > not being printed at all.
> > ...
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > builtin/receive-pack.c | 26 ++++++++++++--------------
> > 1 file changed, 12 insertions(+), 14 deletions(-)
>
> Nice.
>
> Incidentally, this also shows that the refnames in alternate ref
> namespace will not matter. We are to only show just one of many
> anyway (and that name is masked and replaced with ".have"). Perhaps
> we want to do 04/12 without refname?
I kept the refnames there as a "plausible" thing that a callback might
want. But I have ulterior motives. :)
I have another series which uses alternate refs as part of
check_connected(). Since that function calls rev-list, it needs to add
something like "rev-list --alternate-refs". Even check_connected() does
not care about the refnames, it seems like that rev-list option should
have some useful name for each ref. It can make things like "--source"
more sensible, instead of just reporting everything as a blank ".have".
I could be persuaded otherwise, though. I don't think we pay a huge for
the refnames here. The generating for-each-ref has them either way, and
the receiving side only keeps one in memory at a time. So we are really
only paying to send them across the pipe and parse them, which doesn't
seem extravagant. I didn't actually measure it, though.
-Peff
^ permalink raw reply
* Re: [PATCH v2 25/27] attr: store attribute stack in attr_check structure
From: Brandon Williams @ 2017-01-25 19:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, pclouds
In-Reply-To: <xmqqinp5p4x2.fsf@gitster.mtv.corp.google.com>
On 01/23, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > ... It seems like breaking the question and answer up
> > doesn't buy you much in terms of reducing allocation churn and instead
> > complicates the API with needing to keep track of two structures instead
> > of a one.
>
> In my mind, the value of having a constant check_attr is primarily
> that it gives us a stable pointer to serve as a hashmap key,
> i.e. the identifier for each call site, in a later iteration.
We didn't really discuss this notion of having the pointer be a key into
a hashmap, what sort of information are you envisioning being stored in
this sort of hashmap? One issue I can see with this is that the
functions which have a static attr_check struct would still not be thread
safe if the initialization of the structure isn't surrounded by a mutex
itself. ie
static struct attr_check *check;
if (!check)
init(check);
would need to be:
lock()
if (!check)
init(check);
unlock();
inorder to prevent a race to initialize the structure. Which is
something that the attr system itself can't be refactored to fix (at
least I can't see how at the moment).
> Of course, in order to populate the "question" array, we'd need the
> interning of attribute names to attr objects, which need to be
> protected by mutex, and you would probably not want to do that every
> time the control hits the codepath.
While true that doesn't prevent the mutex needed to create/check that
the all_attr array that is used to collect attributes is the correct
size/initialized properly.
> But all of the above comes from my intuition, and I'll very much
> welcome to be proven wrong with an alternative design, or better
> yet, a working code based on an alternative design ;-).
Yeah, after working through the problem the two simple solutions I can
think of are either my v1 or v2 of the series, neither of which allows
for the attr_check structure to be shared. If we truly want the
"question" array to be const then that can be done, it would just
require a bit more boilerplate and making the all_attr array to be
local to the check_attrs() function itself. An API like this would look
like:
static const struct attr_check *check;
struct attr_result result;
if (!check)
init_check(check);
// Result struct needs to be initialized based on the size of check
init_result(&result, check);
check_attrs(path, check, &result);
// use result
attr_result_clear(&result);
>
It still doesn't handle an initialization race on the check structure
but the check pointer would be const and could be used for some future
optimization. It also will have a bit more allocation churn than either
v1 or v2 of the series. If this is the route you want to take I'll get
working on it, I just want to make sure we're on the same page before
doing a larger refactor like this.
Thanks for the guidance on this, someday we'll get this right :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 11/12] receive-pack: treat namespace .have lines like alternates
From: Jeff King @ 2017-01-25 19:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa8aec40a.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 25, 2017 at 11:51:17AM -0800, Junio C Hamano wrote:
> This is an unrelated tangent, but there may want to be a knob to
> make the code return here without even showing, to make the
> advertisement even smaller and also to stop miniscule information
> leakage? If the namespaced multiple projects are totally unrelated
> (i.e. "My sysadmin gave me a write access only to this single
> directory, so I am using the namespace feature to host these three
> projects that have nothing to do with each other"), showing objects
> of other namespaces will buy us nothing and the user is better off
> without this code showing these refs as ".have".
Yeah, I'd agree it's a potential issue. And I definitely do the same
with alternates (in my case it isn't "they're not relevant" as much as
"they are too big, and the optimization backfires").
I don't use namespaces myself[1], though, so I'd prefer to leave that to
somebody who has that itch, and could think it through better.
-Peff
[1] They _seem_ like they'd be a good fit for the way GitHub uses
alternates, but since they're only implemented for fetch/push,
they're only a small part of the story. Plus the ref storage has
traditionally been a scaling bottleneck for us, so it's nice for
each fork to have its own ref storage for operations that just touch
that fork.
^ permalink raw reply
* Re: [PATCH 12/12] receive-pack: avoid duplicates between our refs and alternates
From: Junio C Hamano @ 2017-01-25 20:02 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170124004818.7resjwbe6ldqjfyx@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> We de-duplicate ".have" refs among themselves, but never
> check if they are duplicates of our local refs. It's not
> unreasonable that they would be if we are a "--shared" or
> "--reference" clone of a similar repository; we'd have all
> the same tags.
>
> We can handle this by inserting our local refs into the
> oidset, but obviously not suppressing duplicates (since the
> refnames are important).
Makes sense.
> +extract_ref_advertisement () {
> + perl -lne '
> + # \\ is there to skip capabilities after \0
> + /push< ([^\\]+)/ or next;
> + exit 0 if $1 eq "0000";
> + print $1;
> + '
Parsing TRACE_PACKET output? Yuck. But I think this has to do, as
any other solution will bound to be uglier.
> +test_expect_success 'receive-pack de-dupes .have lines' '
> + git init shared &&
> + git -C shared commit --allow-empty -m both &&
> + git clone -s shared fork &&
> + (
> + cd shared &&
> + git checkout -b only-shared &&
> + git commit --allow-empty -m only-shared &&
> + git update-ref refs/heads/foo HEAD
> + ) &&
> +
> + # Notable things in this expectation:
> + # - local refs are not de-duped
> + # - .have does not duplicate locals
> + # - .have does not duplicate itself
> + local=$(git -C fork rev-parse HEAD) &&
> + shared=$(git -C shared rev-parse only-shared) &&
> + cat >expect <<-EOF &&
> + $local refs/heads/master
> + $local refs/remotes/origin/HEAD
> + $local refs/remotes/origin/master
> + $shared .have
> + EOF
We may want to sort this thing and the extracted one when comparing;
the order of the entries is not part of the feature we cast in stone.
> +
> + GIT_TRACE_PACKET=$(pwd)/trace git push fork HEAD:foo &&
> + extract_ref_advertisement <trace >refs &&
> + test_cmp expect refs
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH 12/12] receive-pack: avoid duplicates between our refs and alternates
From: Jeff King @ 2017-01-25 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq60l2c3hl.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 25, 2017 at 12:02:30PM -0800, Junio C Hamano wrote:
> > +extract_ref_advertisement () {
> > + perl -lne '
> > + # \\ is there to skip capabilities after \0
> > + /push< ([^\\]+)/ or next;
> > + exit 0 if $1 eq "0000";
> > + print $1;
> > + '
>
> Parsing TRACE_PACKET output? Yuck. But I think this has to do, as
> any other solution will bound to be uglier.
Agreed. My initial attempt was to just run "git receive-pack </dev/null"
and parse the advertisement. But then you have to parse pktlines. :)
> > + # Notable things in this expectation:
> > + # - local refs are not de-duped
> > + # - .have does not duplicate locals
> > + # - .have does not duplicate itself
> > + local=$(git -C fork rev-parse HEAD) &&
> > + shared=$(git -C shared rev-parse only-shared) &&
> > + cat >expect <<-EOF &&
> > + $local refs/heads/master
> > + $local refs/remotes/origin/HEAD
> > + $local refs/remotes/origin/master
> > + $shared .have
> > + EOF
>
> We may want to sort this thing and the extracted one when comparing;
> the order of the entries is not part of the feature we cast in stone.
True (and in fact the .have moved in the previous patch). I wondered,
though, if it might not be a reasonable thing for somebody to have to
look at the output and verify it when the order _does_ change.
I dunno.
-Peff
^ permalink raw reply
* Re: [PATCH v2 25/27] attr: store attribute stack in attr_check structure
From: Stefan Beller @ 2017-01-25 20:10 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <20170125195745.GA83343@google.com>
On Wed, Jan 25, 2017 at 11:57 AM, Brandon Williams <bmwill@google.com> wrote:
> On 01/23, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>> > ... It seems like breaking the question and answer up
>> > doesn't buy you much in terms of reducing allocation churn and instead
>> > complicates the API with needing to keep track of two structures instead
>> > of a one.
>>
>> In my mind, the value of having a constant check_attr is primarily
>> that it gives us a stable pointer to serve as a hashmap key,
>> i.e. the identifier for each call site, in a later iteration.
>
> We didn't really discuss this notion of having the pointer be a key into
> a hashmap, what sort of information are you envisioning being stored in
> this sort of hashmap? One issue I can see with this is that the
> functions which have a static attr_check struct would still not be thread
> safe if the initialization of the structure isn't surrounded by a mutex
> itself. ie
>
> static struct attr_check *check;
>
> if (!check)
> init(check);
>
> would need to be:
>
> lock()
> if (!check)
> init(check);
> unlock();
>
> inorder to prevent a race to initialize the structure. Which is
> something that the attr system itself can't be refactored to fix (at
> least I can't see how at the moment).
By passing the check pointer into the attr system (using a double pointer)
extern void git_attr_check_initl( \
struct git_attr_check out**, \
const char *, ...)
{
// get the global lock, as construction of new check structs
// is not expected to produce contention
// parse the list of things & construct the thing
*out = /* I made a thing */
// unlock globally
}
>
>> Of course, in order to populate the "question" array, we'd need the
>> interning of attribute names to attr objects, which need to be
>> protected by mutex, and you would probably not want to do that every
>> time the control hits the codepath.
>
> While true that doesn't prevent the mutex needed to create/check that
> the all_attr array that is used to collect attributes is the correct
> size/initialized properly.
>
>> But all of the above comes from my intuition, and I'll very much
>> welcome to be proven wrong with an alternative design, or better
>> yet, a working code based on an alternative design ;-).
>
> Yeah, after working through the problem the two simple solutions I can
> think of are either my v1 or v2 of the series, neither of which allows
> for the attr_check structure to be shared. If we truly want the
> "question" array to be const then that can be done, it would just
> require a bit more boilerplate and making the all_attr array to be
> local to the check_attrs() function itself. An API like this would look
> like:
>
> static const struct attr_check *check;
> struct attr_result result;
>
> if (!check)
> init_check(check);
>
> // Result struct needs to be initialized based on the size of check
> init_result(&result, check);
Behind the scenes we may have a pool that caches result allocations,
such that we avoid memory allocation churn in here
>
> check_attrs(path, check, &result);
>
> // use result
>
> attr_result_clear(&result);
Instead of clearing here, we'd give it back to the pool, which then can keep
parts of the result intact.
^ permalink raw reply
* Re: [PATCH v2 25/27] attr: store attribute stack in attr_check structure
From: Junio C Hamano @ 2017-01-25 20:14 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, pclouds
In-Reply-To: <20170125195745.GA83343@google.com>
Brandon Williams <bmwill@google.com> writes:
>> In my mind, the value of having a constant check_attr is primarily
>> that it gives us a stable pointer to serve as a hashmap key,
>> i.e. the identifier for each call site, in a later iteration.
>
> We didn't really discuss this notion of having the pointer be a key into
> a hashmap, what sort of information are you envisioning being stored in
> this sort of hashmap?
The "entries relevant to this attr_check() call, that is specific to
the <check_attr instance, the thread> tuple" (aka "what used to be
called the global attr_stack") we discussed would be the primary
example. A thread is likely be looping in a caller that has many
paths inside a directory, calling a function that has a call to
attr_check() for each path. Having something that can use to
identify the check_attr instance in a stable way, even when the
inner function is called and returns many times, would allow us to
populate the "attr stack" just once for the thread when it enters a
directory for the first time (remember, another thread may be
executing the same codepath, checking for paths in a different
directory) and keep using it. There may be other mechanisms you can
come up with, so I wouldn't say it is the only valid way, but it is
a way. That is why I said:
>> But all of the above comes from my intuition, and I'll very much
>> welcome to be proven wrong with an alternative design, or better
>> yet, a working code based on an alternative design ;-).
near the end of my message.
> One issue I can see with this is that the
> functions which have a static attr_check struct would still not be thread
> safe if the initialization of the structure isn't surrounded by a mutex
> itself. ie
Yes, that goes without saying. That is why I suggested Stefan to do
not this:
> static struct attr_check *check;
>
> if (!check)
> init(check);
>
> would need to be:
>
> lock()
> if (!check)
> init(check);
> unlock();
but this:
static struct attr_check *check;
init(&check);
and hide the lock/unlock gymnastics inside the API. I thought that
already was in what you inherited from him and started your work
on top of?
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
From: Junio C Hamano @ 2017-01-25 20:35 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZKDyfP1SPsxAf8oMSU3763QiogLpUzFZHy+OTQQdJP6A@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Mon, Jan 23, 2017 at 4:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> * sb/unpack-trees-super-prefix (2017-01-12) 5 commits
>> - SQUASH
>> - unpack-trees: support super-prefix option
>> - t1001: modernize style
>> - t1000: modernize style
>> - read-tree: use OPT_BOOL instead of OPT_SET_INT
>>
>> "git read-tree" and its underlying unpack_trees() machinery learned
>> to report problematic paths prefixed with the --super-prefix option.
>>
>> Expecting a reroll.
>> The first three are in good shape. The last one needs a better
>> explanation and possibly an update to its test.
>> cf. <CAGZ79kaHDnVLQr8WLoaD5KKs7EqeW=KbkptF=iHpU5t054Xcdw@mail.gmail.com>
>>
>
> Please see
> https://public-inbox.org/git/20170118010520.8804-1-sbeller@google.com/
Thanks, applied. Let's move this forward.
^ permalink raw reply
* Re: GSoC 2017: application open, deadline = February 9, 2017
From: Jeff King @ 2017-01-25 20:45 UTC (permalink / raw)
To: Matthieu Moy
Cc: git, Pranit Bauva, Lars Schneider, Christian Couder,
Carlos Martín Nieto, Johannes Schindelin, Thomas Gummerer
In-Reply-To: <vpq1svtstud.fsf@anie.imag.fr>
On Mon, Jan 23, 2017 at 04:02:02PM +0100, Matthieu Moy wrote:
> If the answer to one of the above question is yes, then:
>
> * Who's willing to mentor? and to admin?
I did co-admin last year and the year before, but I made Matthieu do all
the work. :)
I do not mind doing the administrative stuff. But the real work is in
polishing up the ideas list and microprojects page. So I think the first
step, if people are interested in GSoC, is not just to say "yes, let's
do it", but to actually flesh out these pages:
> * We need to write the application, i.e. essentially polish and update
> the text here: https://git.github.io/SoC-2016-Org-Application/ and
> update the list of project ideas and microprojects :
> https://git.github.io/SoC-2017-Ideas/
> https://git.github.io/SoC-2016-Microprojects/
That can be done incrementally by people who care (especially mentors)
over the next week or so, and doesn't require any real admin
coordination. If it happens and the result looks good, then the
application process is pretty straightforward.
If it doesn't, then we probably ought not to participate in GSoC.
-Peff
PS If we do participate, we should consider Outreachy as well, which can
expand our base of possible applicants. Though note that ones who are
not eligible for GSoC would need to be funded by us. Last year GitHub
offered to cover the stipend for an Outreachy intern for Git. If
there's interest I can see if that offer can be extended for this
year.
^ permalink raw reply
* Re: [PATCH 2/5] revision.c: group ref selection options together
From: Jeff King @ 2017-01-25 20:50 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, jacob.keller
In-Reply-To: <20170125125054.7422-3-pclouds@gmail.com>
On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:
> These options have on thing in common: when specified right after
> --exclude, they will de-select refs instead of selecting them by
> default.
>
> This change makes it possible to introduce new options that use these
> options in the same way as --exclude. Such an option would just
> implement something like handle_refs_pseudo_opt().
>
> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
> that similar functions like handle_refs_pseudo_opt() are forced to
> handle all ref selector options, not skipping some by mistake, which may
> revert the option back to default behavior (rev selection).
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> revision.c | 134 +++++++++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 100 insertions(+), 34 deletions(-)
Hmm. I see what you're trying to do here, and abstract the repeated
bits. But I'm not sure the line-count reflects a real simplification.
Everything ends up converted to an enum, and then that enum just expands
to similar C code.
I kind of expected that clear_ref_exclusion() would just become a more
abstract clear_ref_selection(). For now it would clear exclusions, and
then later learn to clear the decoration flags.
Maybe I am missing something in the later patches, though.
-Peff
^ permalink raw reply
* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Junio C Hamano @ 2017-01-25 20:52 UTC (permalink / raw)
To: Christian Couder
Cc: Duy Nguyen, git, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD3KXOgVOhuMtws36XPiek56U4mQKdUs07hzKc-dC=ppmA@mail.gmail.com>
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.
> 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". 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.
> 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.
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), 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". 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.
[Footnote]
*1* My understanding is that we lose information on stale entries in
the shared file that are covered by the split index overlay
after read_index() returns, so we _might_ be able to write the
"old" one that is sufficient for _our_ split index, but we do
not have good enough information to recreate "old" one usable by
other split index files.
^ permalink raw reply
* Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude
From: Jeff King @ 2017-01-25 20:57 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, jacob.keller
In-Reply-To: <20170125125054.7422-5-pclouds@gmail.com>
On Wed, Jan 25, 2017 at 07:50:53PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
>
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
>
> --exclude .. --decorate-reflog ... --remotes
>
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?
I don't think it means either. It means to include remotes in the
selected revisions, but excluding the entries mentioned by --exclude.
IOW:
--exclude=foo --remotes
include all remotes except refs/remotes/foo
--exclude=foo --unrelated --remotes
same
--exclude=foo --decorate-reflog --remotes
decorate reflogs of all remotes except "foo". Do _not_ use them
as traversal tips.
--decorate-reflog --exclude=foo --remotes
same
IOW, the ref-selector options build up until a group option is given,
which acts on the built-up options (over that group) and then resets the
built-up options. Doing "--unrelated" as above is orthogonal (though I
think in practice nobody would do that, because it's hard to read).
> Granted, there may be valid use cases for such a combination (e.g.
> "decorate all reflogs except remote ones") but I feel option order is
> not a good fit to express them.
That would be spelled:
--exclude=refs/remotes --decorate-reflogs --all
(or you could swap the first two options).
Again, I'm not sure if I'm missing something subtle, or if you are
confused about how --exclude works. :)
-Peff
^ permalink raw reply
* Re: [PATCH 06/12] clone: disable save_commit_buffer
From: Jeff King @ 2017-01-25 21:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20170125193541.yxqq4avnjngbigmq@sigill.intra.peff.net>
On Wed, Jan 25, 2017 at 02:35:41PM -0500, Jeff King wrote:
> On Wed, Jan 25, 2017 at 02:27:40PM -0500, Jeff King wrote:
>
> > > > For fetching operations like clone, we already disable
> > >
> > > s/clone/fetch/ you meant?
> >
> > Well, no, because this patch deals with clone.
> >
> > It's likely that builtin/fetch.c would want the same treatment. It
> > didn't come up for me because I've disabled the alternates check for
> > that case (but you can't do that with stock git), and I didn't dig
> > further.
> >
> > Possibly this should just go into fetch-pack.c, right before we walk
> > over all of the refs and call parse_object() and deref_tag() on all of
> > them. It feels a little funny to tweak the global save_commit_buffer
> > flag there, but it already do so in everything_local(), so I don't think
> > we're really losing much.
>
> Hrm. So I thought it might be useful to write a patch that just tweaks
> save_commit_buffer at the start of fetch_pack(). But looking it over,
> we call everything_local() _before_ we walk over all the refs. So
> save_commit_buffer should already be turned off for my case.
>
> I wonder if I made a mistake while measuring the peak RSS. Or if clone
> parses some commits before it calls into fetch_pack() (but which
> objects? It doesn't have any until it does the fetch).
>
> Perhaps we should just drop this patch (though I think it is logically
> consistent and wouldn't hurt anything).
OK, I just repeated my heap measurements with valgrind's massif tool,
specifically checking builds of this patch and its parent. I couldn't
find any improvement. So I must have screwed something up in my earlier
measurements.
Sorry for the noise. I think we should probably just drop this patch.
-Peff
^ permalink raw reply
* Re: [PATCH 2/5] revision.c: group ref selection options together
From: Junio C Hamano @ 2017-01-25 21:11 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, jacob.keller
In-Reply-To: <20170125125054.7422-3-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> These options have on thing in common: when specified right after
one thing.
> --exclude, they will de-select refs instead of selecting them by
> default.
>
> This change makes it possible to introduce new options that use these
> options in the same way as --exclude. Such an option would just
> implement something like handle_refs_pseudo_opt().
>
> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
> that similar functions like handle_refs_pseudo_opt() are forced to
> handle all ref selector options, not skipping some by mistake, which may
> revert the option back to default behavior (rev selection).
I am not sure about these two refactorings, for at least two reasons.
* Naming. The function is all about handling "refs options" and I
do not see anything "pseudo" about the options handled by the
handle_refs_pseudo_opt() function. This is minor.
* I am expecting that the new one yet to be introduced will not
share the huge "switch (selector)" part, but does its own things
in a separate function with a similar structure. The only thing
common between these two functions would be the structure
(i.e. it has a big "switch(selector)" that does different things
depending on REF_SELECT_*) and a call to clear_* function.
If we were to add a new kind of REF_SELECT_* (say
REF_SELECT_NOTES just for the sake of being concrete), what
changes will be needed to the code if the addition of "use reflog
from this class of refs for decoration" feature was done with or
without this step? I have a suspicion that the change will be
simpler without this step.
The above comments will be invalid if the new "use reflog for
decoration" feature will be done by extending this pseudo_opt()
function by extending each of the switch/case arms. If that is
the case, please disregard the above. I just didn't get that
impression from the above paragraph.
^ permalink raw reply
* Re: [PATCH] Retire the `relink` command
From: Jeff King @ 2017-01-25 21:13 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <10319c47ff3f7222c3a601827ebd9398861d509d.1485363528.git.johannes.schindelin@gmx.de>
On Wed, Jan 25, 2017 at 05:58:57PM +0100, Johannes Schindelin wrote:
> Back in the olden days, when all objects were loose and rubber boots were
> made out of wood, it made sense to try to share (immutable) objects
> between repositories.
>
> Ever since the arrival of pack files, it is but an anachronism.
Yes, this is a good idea. I could _almost_ see its utility if it linked
packfiles, too, but then it is very unlikely that two repos have the
exact same packfile.
> Let's move the script to the contrib/examples/ directory and no longer
> offer it.
I am OK with this, but perhaps we should go even further and just delete
it entirely. The point of contrib/examples is to show people "this is
how you could script plumbing to implement a porcelain". But this script
does not call a single plumbing script. It just looks directly in
objects/, which is probably not something we would want to encourage. :)
-Peff
^ permalink raw reply
* Re: [PATCH] mingw: allow hooks to be .exe files
From: Jeff King @ 2017-01-25 21:15 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <dc388b2df3baee83972f007cd77a57410553a411.1485362812.git.johannes.schindelin@gmx.de>
On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
> This change is necessary to allow the files in .git/hooks/ to optionally
> have the file extension `.exe` on Windows, as the file names are
> hardcoded otherwise.
Make sense as a goal.
> - if (access(path.buf, X_OK) < 0)
> + if (access(path.buf, X_OK) < 0) {
> +#ifdef STRIP_EXTENSION
> + strbuf_addstr(&path, ".exe");
I think STRIP_EXTENSION is a string. Should this line be:
strbuf_addstr(&path, STRIP_EXTENSION);
?
-Peff
^ permalink raw reply
* Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude
From: Junio C Hamano @ 2017-01-25 21:15 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, jacob.keller
In-Reply-To: <20170125125054.7422-5-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
>
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
>
> --exclude .. --decorate-reflog ... --remotes
>
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?
I would expect that the effect of exclude, decorate-reflog and
friends will extend until the next occurrence of --remotes (or --all
or whatever you catch in parse_ref_selector_option() function).
So, I'd read it as "add all remote tracking refs, but (1) exclude
exclude the refs matching pattern .. and (2) use reflog of them if
they match pattern ...".
Or did you mean by "..." something other than a single argument that
is a pattern?
^ permalink raw reply
* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
From: Junio C Hamano @ 2017-01-25 21:20 UTC (permalink / raw)
To: Jeff King
Cc: Thomas Gummerer, git, Stephan Beyer, Marc Strapetz,
Johannes Schindelin
In-Reply-To: <20170124201415.zzyg4vt35vflwjnb@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Jan 21, 2017 at 08:08:02PM +0000, Thomas Gummerer wrote:
>
>> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
>> index 2e9cef06e6..0ad5335a3e 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 revert the
>> + the changes in the working tree to match the index.
>> + The <message> part is optional and gives
>
> Hrm. "git reset --hard" doesn't just make the working tree match the
> index. It also resets the index to HEAD. So either the original or your
> new description is wrong.
>
> I think it's the latter. We really do reset the index unless
> --keep-index is specified.
Correct. So the patch is a net loss. Perhaps not requiring readers
to know "reset --hard" might be an improvement (I personally do not
think so), but this loses crucial information from the description.
Save your local modifications (both in the working tree and
to the index) to a new 'stash', and resets the index to HEAD
and makes the working tree match the index (i.e. runs "git
reset --hard").
That's one-and-a-half lines longer than the original, though.
^ permalink raw reply
* Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard
From: Junio C Hamano @ 2017-01-25 21:20 UTC (permalink / raw)
To: Jakub Narębski
Cc: Jeff King, Thomas Gummerer, git, Stephan Beyer, Marc Strapetz,
Johannes Schindelin
In-Reply-To: <d327cfc5-2f0d-1a67-d5f5-b9b7a06f7570@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
>>> - 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 revert the
>>> + the changes in the working tree to match the index.
>>> + The <message> part is optional and gives
>>
>> Hrm. "git reset --hard" doesn't just make the working tree match the
>> index. It also resets the index to HEAD. So either the original or your
>> new description is wrong.
>>
>> I think it's the latter. We really do reset the index unless
>> --keep-index is specified.
>
> I wonder if it is worth mentioning that "saving local modifications"
> in 'git stash' means storing both the state of the worktree (of tracked
> files in it) _and_ the state of the index, before both get set to
> state of HEAD.
Yes, it is, I would say.
^ permalink raw reply
* Re: [PATCH] tag: add tag.createReflog option
From: Cornelius Weig @ 2017-01-25 21:21 UTC (permalink / raw)
To: Jeff King; +Cc: git, novalis, pclouds
In-Reply-To: <20170125180054.7mioop2o6uvqloyt@sigill.intra.peff.net>
On 01/25/2017 07:00 PM, Jeff King wrote:
> - Is that the end of it, or is the desire really "I want reflogs for
> _everything_"? That seems like a sane thing to want.
>
> If so, then the update to core.logallrefupdates should turn it into
> a tri-state:
>
> - false; no reflogs
>
> - true; reflogs for branches, remotes, notes, as now
>
> - always; reflogs for all refs under "refs/"
>
I think you nailed it. This is much more useful than what I suggested.
I'll see if I can code it up.
^ permalink raw reply
* Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude
From: Jeff King @ 2017-01-25 21:27 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, jacob.keller
In-Reply-To: <20170125205718.ksqstdnazmgbkehy@sigill.intra.peff.net>
On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:
> IOW, the ref-selector options build up until a group option is given,
> which acts on the built-up options (over that group) and then resets the
> built-up options. Doing "--unrelated" as above is orthogonal (though I
> think in practice nobody would do that, because it's hard to read).
So here's what I would have expected your series to look more like (with
probably one patch adding clear_ref_selection_options, and the other
adding the decorate stuff):
diff --git a/revision.c b/revision.c
index b37dbec37..2f67707c7 100644
--- a/revision.c
+++ b/revision.c
@@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
if (ref_excluded(cb->all_revs->ref_excludes, path))
return 0;
+ if (cb->all_revs->decorate_reflog) {
+ /* TODO actually do it for real */
+ warning("would decorate %s", path);
+ return 0; /* do not add it as a tip */
+ }
object = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);
add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);
@@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)
string_list_append(*ref_excludes_p, exclude);
}
+static void clear_ref_selection_options(struct rev_info *revs)
+{
+ clear_ref_exclusion(&revs->ref_excludes);
+ revs->decorate_reflog = 0;
+}
+
static void handle_refs(const char *submodule, struct rev_info *revs, unsigned flags,
int (*for_each)(const char *, each_ref_fn, void *))
{
@@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char *submodule,
if (!strcmp(arg, "--all")) {
handle_refs(submodule, revs, *flags, for_each_ref_submodule);
handle_refs(submodule, revs, *flags, head_ref_submodule);
- clear_ref_exclusion(&revs->ref_excludes);
+ clear_ref_selection_options(revs);
} else if (!strcmp(arg, "--branches")) {
handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);
- clear_ref_exclusion(&revs->ref_excludes);
+ clear_ref_selection_options(revs);
} else if (!strcmp(arg, "--bisect")) {
read_bisect_terms(&term_bad, &term_good);
handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
@@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char *submodule,
revs->bisect = 1;
} else if (!strcmp(arg, "--tags")) {
handle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);
- clear_ref_exclusion(&revs->ref_excludes);
+ clear_ref_selection_options(revs);
} else if (!strcmp(arg, "--remotes")) {
handle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);
- clear_ref_exclusion(&revs->ref_excludes);
+ clear_ref_selection_options(revs);
} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
struct all_refs_cb cb;
init_all_refs_cb(&cb, revs, *flags);
for_each_glob_ref(handle_one_ref, optarg, &cb);
- clear_ref_exclusion(&revs->ref_excludes);
+ clear_ref_selection_options(revs);
return argcount;
} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
add_ref_exclusion(&revs->ref_excludes, optarg);
@@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char *submodule,
struct all_refs_cb cb;
init_all_refs_cb(&cb, revs, *flags);
for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
- clear_ref_exclusion(&revs->ref_excludes);
+ clear_ref_selection_options(revs);
} else if (starts_with(arg, "--tags=")) {
struct all_refs_cb cb;
init_all_refs_cb(&cb, revs, *flags);
for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
- clear_ref_exclusion(&revs->ref_excludes);
+ clear_ref_selection_options(revs);
} else if (starts_with(arg, "--remotes=")) {
struct all_refs_cb cb;
init_all_refs_cb(&cb, revs, *flags);
for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
- clear_ref_exclusion(&revs->ref_excludes);
+ clear_ref_selection_options(revs);
+ } else if (!strcmp(arg, "--decorate-reflog")) {
+ revs->decorate_reflog = 1;
} else if (!strcmp(arg, "--reflog")) {
add_reflogs_to_pending(revs, *flags);
} else if (!strcmp(arg, "--indexed-objects")) {
diff --git a/revision.h b/revision.h
index 9fac1a607..c74879829 100644
--- a/revision.h
+++ b/revision.h
@@ -66,6 +66,8 @@ struct rev_info {
/* excluding from --branches, --refs, etc. expansion */
struct string_list *ref_excludes;
+ int decorate_reflog;
+
/* Basic information */
const char *prefix;
const char *def;
^ permalink raw reply related
* Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Junio C Hamano @ 2017-01-25 21:28 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, David Aguilar, Ramsay Jones,
GIT Mailing-list
In-Reply-To: <20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The only advantage is that it is self-documenting, so somebody does not
> come through later and convert ("%s", "") back to (""). We could also
> write a comment. But I am happy if we simply catch it in review (or
> preferably the person is clueful enough to read the output of git-blame
> and see why it is that way in the first place).
And the last sentence unfortunatly does not reflect reality.
I would prefer something self-documenting, like your wrapper with a
comment. Then somebody who is looking at the implementation of
warning_blank_line() will not get tempted to turn "%s", "" into ""
because of the comment. And somebody who is looking at the callsite
of warning_blank_line() will think twice before suggesting to turn
it into warning("").
That does not make it unnecessary to review; we still need to catch
those who wants to add new calls to warning("") without even knowing
the presence of warning_blank_line(), if the original codepath being
touched does not have any call to it.
> So maybe:
In any case, the patch is a minimum effort band-aid that lets us
punt on the whole issue for now, so I'll queue it as-is.
Thanks.
> -- >8 --
> Subject: [PATCH] difftool: hack around -Wzero-length-format warning
>
> Building with "gcc -Wall" will complain that the format in:
>
> warning("")
>
> is empty. Which is true, but the warning is over-eager. We
> are calling the function for its side effect of printing
> "warning:", even with an empty string.
>
> Our DEVELOPER Makefile knob disables the warning, but not
> everybody uses it. Let's silence the warning in the code so
> that nobody reports it or tries to "fix" it.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/difftool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 42ad9e804..b5e85ab07 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
> warning(_("both files modified: '%s' and '%s'."),
> wtdir.buf, rdir.buf);
> warning(_("working tree file has been left."));
> - warning("");
> + warning("%s", "");
> err = 1;
> } else if (unlink(wtdir.buf) ||
> copy_file(wtdir.buf, rdir.buf, st.st_mode))
^ permalink raw reply
* Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude
From: Jacob Keller @ 2017-01-25 21:30 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, Git mailing list
In-Reply-To: <20170125212721.7tbxkqsdtsv2n5mx@sigill.intra.peff.net>
On Wed, Jan 25, 2017 at 1:27 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:
>
>> IOW, the ref-selector options build up until a group option is given,
>> which acts on the built-up options (over that group) and then resets the
>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>> think in practice nobody would do that, because it's hard to read).
>
> So here's what I would have expected your series to look more like (with
> probably one patch adding clear_ref_selection_options, and the other
> adding the decorate stuff):
>
I agree that this is how I would have expected it to work as well.
Thanks,
Jake
> diff --git a/revision.c b/revision.c
> index b37dbec37..2f67707c7 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
>
> if (ref_excluded(cb->all_revs->ref_excludes, path))
> return 0;
> + if (cb->all_revs->decorate_reflog) {
> + /* TODO actually do it for real */
> + warning("would decorate %s", path);
> + return 0; /* do not add it as a tip */
> + }
>
> object = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);
> add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);
> @@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)
> string_list_append(*ref_excludes_p, exclude);
> }
>
> +static void clear_ref_selection_options(struct rev_info *revs)
> +{
> + clear_ref_exclusion(&revs->ref_excludes);
> + revs->decorate_reflog = 0;
> +}
> +
> static void handle_refs(const char *submodule, struct rev_info *revs, unsigned flags,
> int (*for_each)(const char *, each_ref_fn, void *))
> {
> @@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char *submodule,
> if (!strcmp(arg, "--all")) {
> handle_refs(submodule, revs, *flags, for_each_ref_submodule);
> handle_refs(submodule, revs, *flags, head_ref_submodule);
> - clear_ref_exclusion(&revs->ref_excludes);
> + clear_ref_selection_options(revs);
> } else if (!strcmp(arg, "--branches")) {
> handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);
> - clear_ref_exclusion(&revs->ref_excludes);
> + clear_ref_selection_options(revs);
> } else if (!strcmp(arg, "--bisect")) {
> read_bisect_terms(&term_bad, &term_good);
> handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
> @@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char *submodule,
> revs->bisect = 1;
> } else if (!strcmp(arg, "--tags")) {
> handle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);
> - clear_ref_exclusion(&revs->ref_excludes);
> + clear_ref_selection_options(revs);
> } else if (!strcmp(arg, "--remotes")) {
> handle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);
> - clear_ref_exclusion(&revs->ref_excludes);
> + clear_ref_selection_options(revs);
> } else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
> struct all_refs_cb cb;
> init_all_refs_cb(&cb, revs, *flags);
> for_each_glob_ref(handle_one_ref, optarg, &cb);
> - clear_ref_exclusion(&revs->ref_excludes);
> + clear_ref_selection_options(revs);
> return argcount;
> } else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
> add_ref_exclusion(&revs->ref_excludes, optarg);
> @@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char *submodule,
> struct all_refs_cb cb;
> init_all_refs_cb(&cb, revs, *flags);
> for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
> - clear_ref_exclusion(&revs->ref_excludes);
> + clear_ref_selection_options(revs);
> } else if (starts_with(arg, "--tags=")) {
> struct all_refs_cb cb;
> init_all_refs_cb(&cb, revs, *flags);
> for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
> - clear_ref_exclusion(&revs->ref_excludes);
> + clear_ref_selection_options(revs);
> } else if (starts_with(arg, "--remotes=")) {
> struct all_refs_cb cb;
> init_all_refs_cb(&cb, revs, *flags);
> for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
> - clear_ref_exclusion(&revs->ref_excludes);
> + clear_ref_selection_options(revs);
> + } else if (!strcmp(arg, "--decorate-reflog")) {
> + revs->decorate_reflog = 1;
> } else if (!strcmp(arg, "--reflog")) {
> add_reflogs_to_pending(revs, *flags);
> } else if (!strcmp(arg, "--indexed-objects")) {
> diff --git a/revision.h b/revision.h
> index 9fac1a607..c74879829 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -66,6 +66,8 @@ struct rev_info {
> /* excluding from --branches, --refs, etc. expansion */
> struct string_list *ref_excludes;
>
> + int decorate_reflog;
> +
> /* Basic information */
> const char *prefix;
> const char *def;
^ permalink raw reply
* Re: [PATCH] tag: add tag.createReflog option
From: Jeff King @ 2017-01-25 21:33 UTC (permalink / raw)
To: Cornelius Weig; +Cc: git, novalis, pclouds
In-Reply-To: <00712f81-e0ba-52e6-77bc-095a2ed706c4@tngtech.com>
On Wed, Jan 25, 2017 at 10:21:48PM +0100, Cornelius Weig wrote:
> On 01/25/2017 07:00 PM, Jeff King wrote:
>
> > - Is that the end of it, or is the desire really "I want reflogs for
> > _everything_"? That seems like a sane thing to want.
> >
> > If so, then the update to core.logallrefupdates should turn it into
> > a tri-state:
> >
> > - false; no reflogs
> >
> > - true; reflogs for branches, remotes, notes, as now
> >
> > - always; reflogs for all refs under "refs/"
> >
>
> I think you nailed it. This is much more useful than what I suggested.
> I'll see if I can code it up.
I cheated a little. I actually wrote the "always" patch 3 years ago as
part of another thing I was working on. But in the end I didn't need it,
and never submitted it.
The patch is below for reference. I have no idea whether it even applies
now, let alone runs and does the right thing. But perhaps you can
salvage bits of it (but feel free to ignore it if it makes things
harder).
-- >8 --
From: Jeff King <peff@peff.net>
Date: Mon, 15 Apr 2013 23:31:05 -0400
Subject: [PATCH] teach core.logallrefupdates an "always" mode
When core.logallrefupdates is true, we only create a new
reflog for refs that are under certain well-known
hierarchies. The reason is that we know that some
hierarchies (like refs/tags) do not typically change, and
that unknown hierarchies might not want reflogs at all
(e.g., a hypothetical refs/foo might be meant to change
often and drop old history immediately).
However, sometimes it is useful to override this decision
and simply log for all refs, because the safety and audit
trail is more important than the performance implications of
keeping the log around.
This patch introduces a new "always" mode for the
core.logallrefupdates option which will log updates to
everything under refs/, regardless where in the hierarchy it
is (we still will not log things like ORIG_HEAD and
FETCH_HEAD, which are known to be transient).
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 8 +++++---
branch.c | 2 +-
builtin/checkout.c | 2 +-
builtin/init-db.c | 2 +-
cache.h | 9 ++++++++-
config.c | 7 ++++++-
environment.c | 2 +-
refs.c | 23 +++++++++++++++++------
t/t1400-update-ref.sh | 32 ++++++++++++++++++++++++++++++++
9 files changed, 72 insertions(+), 15 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e37ba94a72..cb72e559ec 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -390,10 +390,12 @@ core.logAllRefUpdates::
"$GIT_DIR/logs/<ref>", by appending the new and old
SHA1, the date/time and the reason of the update, but
only when the file exists. If this configuration
- variable is set to true, missing "$GIT_DIR/logs/<ref>"
+ variable is set to `true`, a missing "$GIT_DIR/logs/<ref>"
file is automatically created for branch heads (i.e. under
- refs/heads/), remote refs (i.e. under refs/remotes/),
- note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
+ `refs/heads/`), remote refs (i.e. under `refs/remotes/`),
+ note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.
+ If it is set to `always`, then a missing reflog is automatically
+ created for any ref under `refs/`.
+
This information can be used to determine what commit
was the tip of a branch "2 days ago".
diff --git a/branch.c b/branch.c
index 2bef1e7e71..c11880b181 100644
--- a/branch.c
+++ b/branch.c
@@ -259,7 +259,7 @@ void create_branch(const char *head,
}
if (reflog)
- log_all_ref_updates = 1;
+ log_all_ref_updates = LOG_REFS_NORMAL;
if (forcing)
snprintf(msg, sizeof msg, "branch: Reset to %s",
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a9c1b5a95f..00e231d83b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -564,7 +564,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
char *ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
temp = log_all_ref_updates;
- log_all_ref_updates = 1;
+ log_all_ref_updates = LOG_REFS_NORMAL;
if (log_ref_setup(ref_name, log_file, sizeof(log_file))) {
fprintf(stderr, _("Can not do reflog for '%s'\n"),
opts->new_orphan_branch);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 78aa3872dd..0ebad0b37d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -264,7 +264,7 @@ static int create_default_files(const char *template_path)
const char *work_tree = get_git_work_tree();
git_config_set("core.bare", "false");
/* allow template config file to override the default */
- if (log_all_ref_updates == -1)
+ if (log_all_ref_updates == LOG_REFS_UNSET)
git_config_set("core.logallrefupdates", "true");
if (prefixcmp(git_dir, work_tree) ||
strcmp(git_dir + strlen(work_tree), "/.git")) {
diff --git a/cache.h b/cache.h
index 2b192d24ac..d2bfabc67f 100644
--- a/cache.h
+++ b/cache.h
@@ -536,7 +536,6 @@ extern int minimum_abbrev, default_abbrev;
extern int ignore_case;
extern int assume_unchanged;
extern int prefer_symlink_refs;
-extern int log_all_ref_updates;
extern int warn_ambiguous_refs;
extern int shared_repository;
extern const char *apply_default_whitespace;
@@ -556,6 +555,14 @@ extern int core_preload_index;
extern int core_apply_sparse_checkout;
extern int precomposed_unicode;
+enum log_refs_config {
+ LOG_REFS_UNSET = -1,
+ LOG_REFS_NONE = 0,
+ LOG_REFS_NORMAL, /* see should_create_reflog for rules */
+ LOG_REFS_ALWAYS
+};
+extern enum log_refs_config log_all_ref_updates;
+
enum branch_track {
BRANCH_TRACK_UNSPECIFIED = -1,
BRANCH_TRACK_NEVER = 0,
diff --git a/config.c b/config.c
index b5696354fa..ffb892c0a0 100644
--- a/config.c
+++ b/config.c
@@ -601,7 +601,12 @@ static int git_default_core_config(const char *var, const char *value)
}
if (!strcmp(var, "core.logallrefupdates")) {
- log_all_ref_updates = git_config_bool(var, value);
+ if (value && !strcasecmp(value, "always"))
+ log_all_ref_updates = LOG_REFS_ALWAYS;
+ else if (git_config_bool(var, value))
+ log_all_ref_updates = LOG_REFS_NORMAL;
+ else
+ log_all_ref_updates = LOG_REFS_NONE;
return 0;
}
diff --git a/environment.c b/environment.c
index 85edd7f95a..1867d31d75 100644
--- a/environment.c
+++ b/environment.c
@@ -19,7 +19,7 @@ int ignore_case;
int assume_unchanged;
int prefer_symlink_refs;
int is_bare_repository_cfg = -1; /* unspecified */
-int log_all_ref_updates = -1; /* unspecified */
+enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
int warn_ambiguous_refs = 1;
int repository_format_version;
const char *git_commit_encoding;
diff --git a/refs.c b/refs.c
index 541fec2065..3cd203ef87 100644
--- a/refs.c
+++ b/refs.c
@@ -1896,7 +1896,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
lock->force_write = 1;
flag = log_all_ref_updates;
- log_all_ref_updates = 0;
+ log_all_ref_updates = LOG_REFS_NONE;
if (write_ref_sha1(lock, orig_sha1, NULL))
error("unable to write current sha1 into %s", oldrefname);
log_all_ref_updates = flag;
@@ -1965,16 +1965,27 @@ static int copy_msg(char *buf, const char *msg)
return cp - buf;
}
+int should_create_reflog(const char *refname)
+{
+ switch (log_all_ref_updates) {
+ case LOG_REFS_ALWAYS:
+ return 1;
+ case LOG_REFS_NORMAL:
+ return !prefixcmp(refname, "refs/heads/") ||
+ !prefixcmp(refname, "refs/remotes/") ||
+ !prefixcmp(refname, "refs/notes/") ||
+ !strcmp(refname, "HEAD");
+ default:
+ return 0;
+ }
+}
+
int log_ref_setup(const char *refname, char *logfile, int bufsize)
{
int logfd, oflags = O_APPEND | O_WRONLY;
git_snpath(logfile, bufsize, "logs/%s", refname);
- if (log_all_ref_updates &&
- (!prefixcmp(refname, "refs/heads/") ||
- !prefixcmp(refname, "refs/remotes/") ||
- !prefixcmp(refname, "refs/notes/") ||
- !strcmp(refname, "HEAD"))) {
+ if (should_create_reflog(refname)) {
if (safe_create_leading_directories(logfile) < 0)
return error("unable to create directory for %s",
logfile);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0bbf..f60196d294 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -302,4 +302,36 @@ test_expect_success \
'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
'test OTHER = $(git cat-file blob "master@{2005-05-26 23:42}:F")'
+test_expect_success 'core.logAllRefUpdates=true does not log refs/foo/' '
+ test_config core.logAllRefUpdates true &&
+ test_commit log-true &&
+ git update-ref -m reflog-message refs/heads/logme HEAD &&
+ git update-ref -m reflog-message refs/foo/logme HEAD &&
+ {
+ echo "refs/heads/logme@{0} reflog-message"
+ } >expect &&
+ {
+ git log -g -1 --format="%gD %gs" refs/heads/logme &&
+ git log -g -1 --format="%gD %gs" refs/foo/logme
+ } >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'core.logAllRefUpdates=always logs refs/foo/' '
+ test_config core.logAllRefUpdates always &&
+ test_commit log-always &&
+ git update-ref -m reflog-message refs/heads/logme HEAD &&
+ git update-ref -m reflog-message refs/foo/logme HEAD &&
+ {
+ echo "refs/heads/logme@{0} reflog-message"
+ echo "refs/foo/logme@{0} reflog-message"
+ } >expect &&
+ {
+ git log -g -1 --format="%gD %gs" refs/heads/logme &&
+ git log -g -1 --format="%gD %gs" refs/foo/logme
+ } >actual &&
+ test_cmp expect actual
+'
+
+
test_done
--
2.11.0.840.gd37c5973a
^ 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