* Re: [PATCH 0/1] Update imap-send.c, fix incompatibilities with OpenSSL 1.1.x
From: Junio C Hamano @ 2020-01-07 19:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Liam Huang via GitGitGadget, Liam Huang, git
In-Reply-To: <nycvar.QRO.7.76.6.2001071944250.46@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I am afraid that I do not know of any means to teach GitGitGadget to make
> that call whether it has seen a consensus.
>
> And I fear that you are asking me to punt back that decision to
> contributors, i.e. put a lot of the burden of knowing how Git
> contributions are expected to progress _away_ from GitGitGadget.
Yes, and that is why I am giving review comments to contributors to
teach how the development community works.
> Of course, I can teach GitGitGadget to not Cc: you. Like, always. Not sure
> that you would like that any better because you would not even be Cc:ed
> once consensus was reached.
That would actually be better. Somebody in the discussion thread
would probably say "This is good enough---send it to the maintainer"
when the topic is ready.
^ permalink raw reply
* Re: [PATCH 1/3] graph: fix case that hit assert()
From: Jeff King @ 2020-01-07 19:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee via GitGitGadget, git, brad, sunshine,
Derrick Stolee
In-Reply-To: <xmqqblrf5azn.fsf@gitster-ct.c.googlers.com>
On Tue, Jan 07, 2020 at 11:21:00AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> Second, the horizontal lines in that first line drop their coloring.
> >> This is due to a use of graph_line_addch() instead of
> >> graph_line_write_column(). Using a ternary operator to pick the
> >> character is nice for compact code, but we actually need a column
> >> to provide the color.
> >
> > It seems like this is a totally separate bug, and could be its own
> > commit?
>
> I think so.
>
> And with that removed, all that remains would be a removal of the
> assert() plus an additional test?
Yes, though note that the color thing is a v2.25 regression as well. So
we'd probably want both of them.
-Peff
^ permalink raw reply
* Re: [PATCH v3 10/15] rebase: add an --am option
From: Elijah Newren @ 2020-01-07 19:26 UTC (permalink / raw)
To: Phillip Wood
Cc: Elijah Newren via GitGitGadget, Git Mailing List,
Johannes Schindelin, Denton Liu, Junio C Hamano, Pavel Roskin,
Alban Gruin, SZEDER Gábor
In-Reply-To: <8f2fa083-114a-011f-1480-ae0ebd67d814@gmail.com>
Hi Phillip,
On Tue, Jan 7, 2020 at 6:43 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> On 24/12/2019 19:54, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Currently, this option doesn't do anything except error out if any
> > options requiring the interactive-backend are also passed. However,
> > when we make the default backend configurable later in this series, this
> > flag will provide a way to override the config setting.
>
> I wonder if we really want to add a new option that we know we don't
> want to keep in the long term. Could we not just default to the
> interactive backend and fallback to the am backend if the user passes
> any of the am-based options on the commandline? i.e. reverse the current
> situation of defaulting to the am backend and selecting the interactive
> backend when the user passes certain options.
Good question. For context, this option served a few purposes:
* Providing an escape hatch during the transition in case the merge
backend doesn't handle everything the am one does in our first
attempts (i.e. similar to how we allowed using the scripted rebase
even after providing a built-in one).
* Makes the documentation much more concise and clear (being able to
specify that various flags "imply --am" is simple, whereas removing
the flag makes me feel I need the full description of --am in each of
the few places a flag that would have implied it).
* Provides an easy way to make the plethora of am-specific rebase
tests use the am-backend.
However, I thought of this option before Junio suggested a
rebase.backend config setting, so we could just rely on that instead.
Thus, getting rid of the "--am" flag in detail would mean:
* I need to redo the test changes in this series to use "-c
rebase.backend=am" instead of "--am"
* It will be slightly harder for users to use the escape hatch in
one-off cases during the transition
* We need to figure out the right way to reword the documentation
The first two are pretty minor, so that probably means I just need to
come up with some good wording for the documentation (suggestions
welcome...)
Elijah
^ permalink raw reply
* Re: [PATCH 1/3] graph: fix case that hit assert()
From: Junio C Hamano @ 2020-01-07 19:21 UTC (permalink / raw)
To: Jeff King
Cc: Derrick Stolee via GitGitGadget, git, brad, sunshine,
Derrick Stolee
In-Reply-To: <20200107153006.GA20591@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> Second, the horizontal lines in that first line drop their coloring.
>> This is due to a use of graph_line_addch() instead of
>> graph_line_write_column(). Using a ternary operator to pick the
>> character is nice for compact code, but we actually need a column
>> to provide the color.
>
> It seems like this is a totally separate bug, and could be its own
> commit?
I think so.
And with that removed, all that remains would be a removal of the
assert() plus an additional test?
>> +test_expect_success 'log --graph with multiple tips' '
>
> This nicely covers the assert() problem. Could we check the same case
> with "--color" and test_decode_color to check the coloring issue (see
> t4214 for some prior art)?
^ permalink raw reply
* Re: [PATCH v3 01/15] rebase: extend the options for handling of empty commits
From: Elijah Newren @ 2020-01-07 19:15 UTC (permalink / raw)
To: Phillip Wood
Cc: Elijah Newren via GitGitGadget, Git Mailing List,
Johannes Schindelin, Phillip Wood, Denton Liu, Junio C Hamano,
Pavel Roskin, Alban Gruin, SZEDER Gábor
In-Reply-To: <404424d7-f520-8f89-efef-ca03e66fcd43@gmail.com>
Hi Phillip,
On Tue, Jan 7, 2020 at 6:37 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> Thanks for working on this series, I think making the sequencer the
> default backend is a good idea. I have a few reservations about this
> path though...
Thanks for the feedback. You've provided some good food for thought.
While responding, I came up with an alternate proposal below which I
think may be more in line with what you want. Let me know...
>
> On 24/12/2019 19:54, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Extend the interactive machinery with the ability to handle the full
> > spread of options for how to handle commits that either start or become
> > empty (by "become empty" I mean the changes in a commit are a subset of
> > changes that exist upstream, so the net effect of applying the commit is
> > no changes). Introduce a new command line flag for selecting the
> > desired behavior:
> > --empty={drop,keep,ask}
> > with the definitions:
> > drop: drop empty commits
> > keep: keep empty commits
> > ask: provide the user a chance to interact and pick what to do with
> > empty commits on a case-by-case basis
>
> I think we want to distinguish between commits that are empty before
> rebasing and those that become empty when they are rebased. --keep-empty
> explicily only applies to commits that are already empty. Cherry-pick
I am open to changing how the empty handling works; I figured it was
the most likely area people might have feedback about.
However, I do strongly disagree with the "explicit" claim here. It is
possible that --keep-empty was designed to only apply to commits that
begin empty, but that is not at all explicit. The only place in the
documentation I could find that mentions the distinction at all was in
the "Behavioral Differences" section, which was written by me as part
of commit 0661e49aeb84 ("git-rebase.txt: document behavioral
differences between modes", 2018-06-27). Quoting from that commit
message:
"(In fact, it's not clear -- to me at least -- that these differences
were even desirable or intentional.) Document these differences."
I cannot find anywhere else making a distinction in either the
documentation or the commit messages or even any code comments, which
has left me wondering whether the distinction was intentional at all.
Things like commit b00bf1c9a8dd ("git-rebase: make
--allow-empty-message the default", 2018-06-27) give heavy precedence
to the assumption that git-rebase is often more happenstance than
design when it comes to edge cases like these.
> has distinct options for those two cases. If I've explicitly created an
> empty commit then I want to keep it but I don't want to keep commits
> that become empty because the changes they contain are already upstream.
>
> If we want an option that keeps commits that become empty (Off hand I
> don't know why we would though) we should consider if that option should
> disable --cherry-mark when we create the todo list so that it keeps all
> commits that become empty when they're rebased.
To answer the particular reason I have seen why folks might want to
keep both commits that start empty and become empty, but still use
--cherry-mark to discard commits that are already upstream:
* If cherry-mark can determine the commit was "already upstream",
then because of how cherry-mark works this means the upstream commit
message was about the *exact* same set of changes. Thus, the commit
messages can be assumed to be fully interchangeable (and are in fact
likely to be completely identical).
* If a commit does not start empty, but becomes so because its
changes were a subset of those upstream, then while there are clearly
no more changes that need to be applied, the commit messages can be
expected to differ -- either there will be an upstream commit that
included a bunch of other changes too, or the commit we are rebasing
will be spread across multiple upstream commits. If the project has
bad commit message hygiene (because e.g. they are using atrocious code
review tools like GitHub that don't allow you to review them), there's
a reasonable likelihood that the upstream version only talks about the
other changes in the commit instead of the fix we are rebasing. If
so, there may be useful information in this commit message that we
want to copy somewhere.
It's a somewhat rare usecase involving projects that generally aren't
careful with commit messages but which has some people working on it
who are careful. But even for these projects, the odds that a commit
message will just get tossed anyway seem fairly high, so I will not
strongly defend this choice; it just seemed like "--keep-empty" or
"--empty=keep" should mean "keep empty", not "sometimes keep empty".
So, if it'd be nice if we could come up with clear wording if we are
going to implement other possibilities.
Anyway, am I correctly understanding that you would like to see the
following matrix of options?
* Drop commits that either start or become empty [the current --empty=drop]
* Pause and ask users whether they want commits that either start or
become empty [the current --empty=ask]
* Keep commits that start empty, but drop ones that become empty [a
new option]
and possibly drop the current --empty=keep behavior?
> > Note that traditionally, am-based rebases have always dropped commits
> > that either started or became empty, while interactive-based rebases
> > have defaulted to ask (and provided an option to keep commits that
> > started empty). This difference made sense since users of an am-based
> > rebase just wanted to quickly batch apply a sequence of commits, while
> > users editing a todo list will likely want the chance to interact and
> > handle unusual cases on a case-by-case basis.
>
> I don't see why it makes sense to drop an empty commit that I've made
> just because it is being rebased.
Are you arguing that keeping commits that started empty should
actually be the default, i.e. that all three rebase backends have
traditionally had the wrong default?
> I'm pretty sure the behavor of the
> am-based rebase is a function of `git am` not being able to create empty
> commits.
That may be, but it's not quite clear. Junio kind of commented on
this in https://lore.kernel.org/git/xmqqfu1fswdh.fsf@gitster-ct.c.googlers.com/.
His comments that "lack of --keep-empty on the 'am' side is probably a
bug that wants to be fixed" might imply that he only sees it as a
useful option. If the behavior of the am-based rebase dropping
commits which started empty was solely a function of git-am not being
able to create empty commits, then the correct fix would be to make
keeping the commits which started empty as the default or maybe only
behavior rather than just making it an option.
I'd actually be open to changing the default here. We have a
precedent in doing so with commit b00bf1c9a8dd ("git-rebase: make
--allow-empty-message the default", 2018-06-27) where we stated that
really rare edge cases (which I think both empty commit messages and
commits which start empty qualify as) probably have the wrong default
in rebase. And changing the default would yield a much easier to
understand matrix of options:
* Drop commits that become empty (modification to --empty=drop in
that it keeps commits which started empty)
* Pause and ask users if commits become empty (modification to
--empty=ask in that it keeps commits which started empty)
* Keep commits that become empty, in addition to those that started
empty (--empty=keep).
> > However, not all rebases
> > using the interactive machinery are explicitly interactive anymore. In
> > particular --merge was always meant to behave more like --am: just
> > rebase a batch of commits without popping up a todo list.
> >
> > If the --empty flag is not specified, pick defaults as follows:
> > explicitly interactive: ask
> > --exec: keep (exec is about checking existing commits, and often
> > used without actually changing the base. Thus the
> > expectation is that the user doesn't necessarily want
> > anything to change; they just want to test).
> > otherwise: drop
>
> I'm not sure I like changing the behavior based on --exec, I see what
> you're getting at but it has the potential to be confusing. What if I
> want to rearrange the commits without changing the base - why must I
> specify --empty=keep there but not if I add --exec to the command line?
This was probably poorly explained and also poorly implemented. Let
me try from a slightly different angle, and come to a slightly
different result than I did previously:
Quoting from Junio in b00bf1c9a8dd ("git-rebase: make
--allow-empty-message the default", 2018-06-27),
'"am" based rebase is solely to transplant an existing history and
want to stop much less than "interactive" one whose purpose is to
polish a series before making it publishable, and asking for
confirmation ("this has become empty--do you want to drop it?") is
more appropriate from the workflow point of view'
I would modify Junio's wording to expand "am based rebase" to say that
if the user hasn't requested an interactive rebase or otherwise
explicitly signalled that they want some measure of interactivity
(e.g. specified --empty=ask), then we should not stop and ask how to
handle commits which started OR became empty. In other worse, it's
not just the am-based rebase that is about transplanting commits but
any rebase where interactivity isn't requested. I think the
traditional --keep-empty is quite broken in this regard since it does
stop and ask with commits that become empty. rebase -m, rebase
--exec, etc. should not stop when a commit becomes empty, there should
be some default it just uses unless the user also adds
--interactive/-i/--empty=ask.
But, if we take your above suggestion to make keeping the commits
which started empty not only the default but remove it from the
options, then this becomes much easier. The choices become (kind of
repeated from above):
[Keep the commits that started empty in all three cases, otherwise:]
* Drop commits that become empty (--empty=drop)
* Pause and ask users if commits become empty (--empty=ask)
* Keep commits that become empty (--empty=keep)
and the defaults are simply:
--interactive/-i: --empty=ask
everything else: --empty=drop
(And as a bonus, in the thread I've referenced twice above Junio
mentioned -- in respect to commits that become empty -- that he thinks
these are the right defaults. He didn't comment on the behavior of
commits that start empty, which seems to be your primary concern.)
> > Also, this commit makes --keep-empty just imply --empty=keep, and hides
> > it from help so that we aren't confusing users with different ways to do
> > the same thing. (I could have added a --drop-empty flag, but then that
> > invites users to specify both --keep-empty and --drop-empty and we have
> > to add sanity checking around that; it seems cleaner to have a single
> > multi-valued option.) This actually fixes --keep-empty too; previously,
> > it only meant to sometimes keep empty commits, in particular commits
> > which started empty would be kept. But it would still error out and ask
> > the user what to do with commits that became empty. Now it keeps empty
> > commits, as instructed.
>
> It certainly changes the behavior of --keep-empty but I'm not sure it
> "fixes" it. If I have some empty commits I want to keep as placeholders
> then that's different from wanting to keep commits that become empty
> because their changes are upstream but --cherry-mark didn't detect them.
>
> In summary I'm in favor of making it easier to drop commits that become
> empty but not tying that to the handling of commits that are empty
> before they are rebased.
Sounds like my modified proposal above may be in line with your tastes?
> I'm also not happy that the deprecation of --keep-empty suddenly makes
> --no-keep-empty an error.
We could translate that to --empty=drop; does that sound reasonable?
[...]
> > @@ -466,7 +494,10 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
> > struct option options[] = {
> > OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
> > REBASE_FORCE),
> > - OPT_BOOL(0, "keep-empty", &opts.keep_empty, N_("keep empty commits")),
> > + { OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
> > + N_("(DEPRECATED) keep empty commits"),
> > + PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN,
>
> It is all very well deprecating --keep-empty but suddenly making
> '--no-keep-empty' an error goes beyond deprecation. Also I'm not sure
> it's worth changing these options as I think the only user is
> git-rebase--preserve-merges.sh
Side track: Since git-rebase--preserve-merges.sh is deprecated and we
want to get rid of it, and rebase-merges exists and is a better
implementation of the original idea, can we just translate rebase -p
into rebase -r and delete git-rebase--preserve-merges.sh? (With a few
wrinkles, such as telling users in the middle of an existing
preserve-merges-rebase that they either need to use an old version of
git to continue their rebase or else abort the rebase?)
> Best Wishes
>
> Phillip
>
[...]
> This function is used by cherry-pick/revert not rebase do we need to
> change it?
Possibly not; I can take a look. I had quite a difficult time trying
to figure out how to make sure that --empty options were saved and
restored across rebase --continue; it's not at all unlikely that I
modified more than I needed to "for consistency".
[...]
> > @@ -3033,9 +3064,15 @@ static int save_opts(struct replay_opts *opts)
>
> again this is for cherry-pick/revert
Okay, will double check this too.
[...]
^ permalink raw reply
* [PATCH 4/4] fsmonitor: update documentation for hook version and watchman hooks
From: Kevin Willford via GitGitGadget @ 2020-01-07 19:04 UTC (permalink / raw)
To: git; +Cc: Kevin Willford, Junio C Hamano, Kevin Willford
In-Reply-To: <pull.510.git.1578423871.gitgitgadget@gmail.com>
From: Kevin Willford <Kevin.Willford@microsoft.com>
A new config value for core.fsmonitorHookVersion was added to be able
to force the version of the fsmonitor hook. Possible values are 1 or 2.
When this is not set the code will use a value of -1 and attempt to use
version 2 of the hook first and if that fails will attempt version 1.
Signed-off-by: Kevin Willford <Kevin.Willford@microsoft.com>
---
Documentation/config/core.txt | 11 +++++++++++
Documentation/githooks.txt | 13 ++++++++++---
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 9e440b160d..74619a9c03 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -68,6 +68,17 @@ core.fsmonitor::
avoiding unnecessary processing of files that have not changed.
See the "fsmonitor-watchman" section of linkgit:githooks[5].
+core.fsmonitorHookVersion::
+ Sets the version of hook that is to be used when calling fsmonitor.
+ There are currently versions 1 and 2. When this is not set,
+ version 2 will be tried first and if it fails then version 1
+ will be tried. Version 1 uses a timestamp as input to determine
+ which files have changes since that time but some monitors
+ like watchman have race conditions when used with a timestamp.
+ Version 2 uses an opaque string so that the monitor can return
+ something that can be used to determine what files have changed
+ without race conditions.
+
core.trustctime::
If false, the ctime differences between the index and the
working tree are ignored; useful when the inode change time
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 50365f2914..3dccab5375 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -490,9 +490,16 @@ fsmonitor-watchman
~~~~~~~~~~~~~~~~~~
This hook is invoked when the configuration option `core.fsmonitor` is
-set to `.git/hooks/fsmonitor-watchman`. It takes two arguments, a version
-(currently 1) and the time in elapsed nanoseconds since midnight,
-January 1, 1970.
+set to `.git/hooks/fsmonitor-watchman` or `.git/hooks/fsmonitor-watchmanv2`
+depending on the version of the hook to use.
+
+Version 1 takes two arguments, a version (1) and the time in elapsed
+nanoseconds since midnight, January 1, 1970.
+
+Version 2 takes two arguments, a version (2) and a token that is used
+for identifying changes since the token. For watchman this would be
+a clock id. This version must output to stdout the new token followed
+by a NUL before the list of files.
The hook should output to stdout the list of all files in the working
directory that may have changed since the requested time. The logic
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/4] fsmonitor: add fsmonitor hook scripts for version 2
From: Kevin Willford via GitGitGadget @ 2020-01-07 19:04 UTC (permalink / raw)
To: git; +Cc: Kevin Willford, Junio C Hamano, Kevin Willford
In-Reply-To: <pull.510.git.1578423871.gitgitgadget@gmail.com>
From: Kevin Willford <Kevin.Willford@microsoft.com>
Version 2 of the fsmonitor hooks is passed the version and an update
token and must pass back a last update token to use for subsequent calls
to the hook.
Signed-off-by: Kevin Willford <Kevin.Willford@microsoft.com>
---
t/t7519/fsmonitor-all-v2 | 21 +++
t/t7519/fsmonitor-watchman-v2 | 173 +++++++++++++++++++++
templates/hooks--fsmonitor-watchman.sample | 138 ++++++++++------
3 files changed, 281 insertions(+), 51 deletions(-)
create mode 100755 t/t7519/fsmonitor-all-v2
create mode 100755 t/t7519/fsmonitor-watchman-v2
diff --git a/t/t7519/fsmonitor-all-v2 b/t/t7519/fsmonitor-all-v2
new file mode 100755
index 0000000000..061907e88b
--- /dev/null
+++ b/t/t7519/fsmonitor-all-v2
@@ -0,0 +1,21 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+#
+# An test hook script to integrate with git to test fsmonitor.
+#
+# The hook is passed a version (currently 2) and since token
+# formatted as a string and outputs to stdout all files that have been
+# modified since the given time. Paths must be relative to the root of
+# the working tree and separated by a single NUL.
+#
+#echo "$0 $*" >&2
+my ($version, $last_update_token) = @ARGV;
+
+if ($version ne 2) {
+ print "Unsupported query-fsmonitor hook version '$version'.\n";
+ exit 1;
+}
+
+print "last_update_token\0/\0"
diff --git a/t/t7519/fsmonitor-watchman-v2 b/t/t7519/fsmonitor-watchman-v2
new file mode 100755
index 0000000000..14ed0aa42d
--- /dev/null
+++ b/t/t7519/fsmonitor-watchman-v2
@@ -0,0 +1,173 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+use IPC::Open2;
+
+# An example hook script to integrate Watchman
+# (https://facebook.github.io/watchman/) with git to speed up detecting
+# new and modified files.
+#
+# The hook is passed a version (currently 2) and last update token
+# formatted as a string and outputs to stdout a new update token and
+# all files that have been modified since the update token. Paths must
+# be relative to the root of the working tree and separated by a single NUL.
+#
+# To enable this hook, rename this file to "query-watchman" and set
+# 'git config core.fsmonitor .git/hooks/query-watchman'
+#
+my ($version, $last_update_token) = @ARGV;
+
+# Uncomment for debugging
+# print STDERR "$0 $version $last_update_token\n";
+
+# Check the hook interface version
+if ($version ne 2) {
+ die "Unsupported query-fsmonitor hook version '$version'.\n" .
+ "Falling back to scanning...\n";
+}
+
+my $git_work_tree = get_working_dir();
+
+my $retry = 1;
+
+my $json_pkg;
+eval {
+ require JSON::XS;
+ $json_pkg = "JSON::XS";
+ 1;
+} or do {
+ require JSON::PP;
+ $json_pkg = "JSON::PP";
+};
+
+launch_watchman();
+
+sub launch_watchman {
+ my $o = watchman_query();
+ if (is_work_tree_watched($o)) {
+ output_result($o->{clock}, @{$o->{files}});
+ }
+}
+
+sub output_result {
+ my ($clockid, @files) = @_;
+
+ # Uncomment for debugging watchman output
+ # open (my $fh, ">", ".git/watchman-output.out");
+ # binmode $fh, ":utf8";
+ # print $fh "$clockid\n@files\n";
+ # close $fh;
+
+ binmode STDOUT, ":utf8";
+ print $clockid;
+ print "\0";
+ local $, = "\0";
+ print @files;
+}
+
+sub watchman_clock {
+ my $response = qx/watchman clock "$git_work_tree"/;
+ die "Failed to get clock id on '$git_work_tree'.\n" .
+ "Falling back to scanning...\n" if $? != 0;
+
+ return $json_pkg->new->utf8->decode($response);
+}
+
+sub watchman_query {
+ my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
+ or die "open2() failed: $!\n" .
+ "Falling back to scanning...\n";
+
+ # In the query expression below we're asking for names of files that
+ # changed since $last_update_token but not from the .git folder.
+ #
+ # To accomplish this, we're using the "since" generator to use the
+ # recency index to select candidate nodes and "fields" to limit the
+ # output to file names only. Then we're using the "expression" term to
+ # further constrain the results.
+ if (substr($last_update_token, 0, 1) eq "c") {
+ $last_update_token = "\"$last_update_token\"";
+ }
+ my $query = <<" END";
+ ["query", "$git_work_tree", {
+ "since": $last_update_token,
+ "fields": ["name"],
+ "expression": ["not", ["dirname", ".git"]]
+ }]
+ END
+
+ # Uncomment for debugging the watchman query
+ # open (my $fh, ">", ".git/watchman-query.json");
+ # print $fh $query;
+ # close $fh;
+
+ print CHLD_IN $query;
+ close CHLD_IN;
+ my $response = do {local $/; <CHLD_OUT>};
+
+ # Uncomment for debugging the watch response
+ # open ($fh, ">", ".git/watchman-response.json");
+ # print $fh $response;
+ # close $fh;
+
+ die "Watchman: command returned no output.\n" .
+ "Falling back to scanning...\n" if $response eq "";
+ die "Watchman: command returned invalid output: $response\n" .
+ "Falling back to scanning...\n" unless $response =~ /^\{/;
+
+ return $json_pkg->new->utf8->decode($response);
+}
+
+sub is_work_tree_watched {
+ my ($output) = @_;
+ my $error = $output->{error};
+ if ($retry > 0 and $error and $error =~ m/unable to resolve root .* directory (.*) is not watched/) {
+ $retry--;
+ my $response = qx/watchman watch "$git_work_tree"/;
+ die "Failed to make watchman watch '$git_work_tree'.\n" .
+ "Falling back to scanning...\n" if $? != 0;
+ $output = $json_pkg->new->utf8->decode($response);
+ $error = $output->{error};
+ die "Watchman: $error.\n" .
+ "Falling back to scanning...\n" if $error;
+
+ # Uncomment for debugging watchman output
+ # open (my $fh, ">", ".git/watchman-output.out");
+ # close $fh;
+
+ # Watchman will always return all files on the first query so
+ # return the fast "everything is dirty" flag to git and do the
+ # Watchman query just to get it over with now so we won't pay
+ # the cost in git to look up each individual file.
+ my $o = watchman_clock();
+ $error = $output->{error};
+
+ die "Watchman: $error.\n" .
+ "Falling back to scanning...\n" if $error;
+
+ output_result($o->{clock}, ("/"));
+ $last_update_token = $o->{clock};
+
+ eval { launch_watchman() };
+ return 0;
+ }
+
+ die "Watchman: $error.\n" .
+ "Falling back to scanning...\n" if $error;
+
+ return 1;
+}
+
+sub get_working_dir {
+ my $working_dir;
+ if ($^O =~ 'msys' || $^O =~ 'cygwin') {
+ $working_dir = Win32::GetCwd();
+ $working_dir =~ tr/\\/\//;
+ } else {
+ require Cwd;
+ $working_dir = Cwd::cwd();
+ }
+
+ return $working_dir;
+}
diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
index ef94fa2938..31d2070b2c 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -8,58 +8,83 @@ use IPC::Open2;
# (https://facebook.github.io/watchman/) with git to speed up detecting
# new and modified files.
#
-# The hook is passed a version (currently 1) and a time in nanoseconds
-# formatted as a string and outputs to stdout all files that have been
-# modified since the given time. Paths must be relative to the root of
-# the working tree and separated by a single NUL.
+# The hook is passed a version (currently 2) and last update token
+# formatted as a string and outputs to stdout a new update token and
+# all files that have been modified since the update token. Paths must
+# be relative to the root of the working tree and separated by a single NUL.
#
# To enable this hook, rename this file to "query-watchman" and set
# 'git config core.fsmonitor .git/hooks/query-watchman'
#
-my ($version, $time) = @ARGV;
+my ($version, $last_update_token) = @ARGV;
# Check the hook interface version
-
-if ($version == 1) {
- # convert nanoseconds to seconds
- # subtract one second to make sure watchman will return all changes
- $time = int ($time / 1000000000) - 1;
-} else {
+if ($version ne 2) {
die "Unsupported query-fsmonitor hook version '$version'.\n" .
"Falling back to scanning...\n";
}
-my $git_work_tree;
-if ($^O =~ 'msys' || $^O =~ 'cygwin') {
- $git_work_tree = Win32::GetCwd();
- $git_work_tree =~ tr/\\/\//;
-} else {
- require Cwd;
- $git_work_tree = Cwd::cwd();
-}
+my $git_work_tree = get_working_dir();
my $retry = 1;
+my $json_pkg;
+eval {
+ require JSON::XS;
+ $json_pkg = "JSON::XS";
+ 1;
+} or do {
+ require JSON::PP;
+ $json_pkg = "JSON::PP";
+};
+
launch_watchman();
sub launch_watchman {
+ $o = watchman_query();
+ if (is_work_tree_watched($o)) {
+ output_result($o->{clock}, @{$o->{files}});
+ }
+}
+
+sub output_result {
+ my ($clockid, @files) = @_;
+
+ binmode STDOUT, ":utf8";
+ print $clockid;
+ print "\0";
+ local $, = "\0";
+ print @files;
+}
+
+sub watchman_clock {
+ my $response = qx/watchman clock "$git_work_tree"/;
+ die "Failed to get clock id on '$git_work_tree'.\n" .
+ "Falling back to scanning...\n" if $? != 0;
+ return $json_pkg->new->utf8->decode($response);
+}
+
+sub watchman_query {
my $pid = open2(\*CHLD_OUT, \*CHLD_IN, 'watchman -j --no-pretty')
- or die "open2() failed: $!\n" .
- "Falling back to scanning...\n";
+ or die "open2() failed: $!\n" .
+ "Falling back to scanning...\n";
# In the query expression below we're asking for names of files that
- # changed since $time but were not transient (ie created after
- # $time but no longer exist).
+ # changed since $last_update_token but not from the .git folder.
#
# To accomplish this, we're using the "since" generator to use the
# recency index to select candidate nodes and "fields" to limit the
- # output to file names only.
-
+ # output to file names only. Then we're using the "expression" term to
+ # further constrain the results.
+ if (substr($last_update_token, 0, 1) eq "c") {
+ $last_update_token = "\"$last_update_token\"";
+ }
my $query = <<" END";
["query", "$git_work_tree", {
- "since": $time,
- "fields": ["name"]
+ "since": $last_update_token,
+ "fields": ["name"],
+ "expression": ["not", ["dirname", ".git"]]
}]
END
@@ -68,24 +93,16 @@ sub launch_watchman {
my $response = do {local $/; <CHLD_OUT>};
die "Watchman: command returned no output.\n" .
- "Falling back to scanning...\n" if $response eq "";
+ "Falling back to scanning...\n" if $response eq "";
die "Watchman: command returned invalid output: $response\n" .
- "Falling back to scanning...\n" unless $response =~ /^\{/;
-
- my $json_pkg;
- eval {
- require JSON::XS;
- $json_pkg = "JSON::XS";
- 1;
- } or do {
- require JSON::PP;
- $json_pkg = "JSON::PP";
- };
-
- my $o = $json_pkg->new->utf8->decode($response);
-
- if ($retry > 0 and $o->{error} and $o->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {
- print STDERR "Adding '$git_work_tree' to watchman's watch list.\n";
+ "Falling back to scanning...\n" unless $response =~ /^\{/;
+
+ return $json_pkg->new->utf8->decode($response);
+}
+
+sub is_work_tree_watched {
+ my ($output) = @_;
+ if ($retry > 0 and $output->{error} and $output->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {
$retry--;
qx/watchman watch "$git_work_tree"/;
die "Failed to make watchman watch '$git_work_tree'.\n" .
@@ -95,15 +112,34 @@ sub launch_watchman {
# return the fast "everything is dirty" flag to git and do the
# Watchman query just to get it over with now so we won't pay
# the cost in git to look up each individual file.
- print "/\0";
+ my $o = watchman_clock();
+ $error = $output->{error};
+
+ die "Watchman: $error.\n" .
+ "Falling back to scanning...\n" if $error;
+
+ output_result($o->{clock}, ("/"));
+ $last_update_token = $o->{clock};
+
eval { launch_watchman() };
- exit 0;
+ return 0;
}
- die "Watchman: $o->{error}.\n" .
- "Falling back to scanning...\n" if $o->{error};
+ die "Watchman: $output->{error}.\n" .
+ "Falling back to scanning...\n" if $output->{error};
- binmode STDOUT, ":utf8";
- local $, = "\0";
- print @{$o->{files}};
+ return 1;
+}
+
+sub get_working_dir {
+ my $working_dir;
+ if ($^O =~ 'msys' || $^O =~ 'cygwin') {
+ $working_dir = Win32::GetCwd();
+ $working_dir =~ tr/\\/\//;
+ } else {
+ require Cwd;
+ $working_dir = Cwd::cwd();
+ }
+
+ return $working_dir;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/4] fsmonitor: handle version 2 of the hooks that will use opaque token
From: Kevin Willford via GitGitGadget @ 2020-01-07 19:04 UTC (permalink / raw)
To: git; +Cc: Kevin Willford, Junio C Hamano, Kevin Willford
In-Reply-To: <pull.510.git.1578423871.gitgitgadget@gmail.com>
From: Kevin Willford <Kevin.Willford@microsoft.com>
Some file monitors like watchman will use something other than a timestamp
to keep better track of what changes happen in between calls to query
the fsmonitor. The clockid in watchman is a string. Now that the index
is storing an opaque token for the last update the code needs to be
updated to pass that opaque token to a verion 2 of the fsmonitor hook.
Because there are repos that already have version 1 of the hook and we
want them to continue to work when git is updated, we need to handle
both version 1 and version 2 of the hook. In order to do that a
config value is being added core.fsmonitorHookVersion to force what
version of the hook should be used. When this is not set it will default
to -1 and then the code will attempt to call version 2 of the hook first.
If that fails it will fallback to trying version 1.
Signed-off-by: Kevin Willford <Kevin.Willford@microsoft.com>
---
fsmonitor.c | 75 +++++++++++++++++++++++++++++++------
t/t7519-status-fsmonitor.sh | 7 +++-
t/t7519/fsmonitor-all | 1 -
t/t7519/fsmonitor-watchman | 3 +-
4 files changed, 71 insertions(+), 15 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index 9860587225..932bd9012d 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -8,7 +8,8 @@
#define INDEX_EXTENSION_VERSION1 (1)
#define INDEX_EXTENSION_VERSION2 (2)
-#define HOOK_INTERFACE_VERSION (1)
+#define HOOK_INTERFACE_VERSION1 (1)
+#define HOOK_INTERFACE_VERSION2 (2)
struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
@@ -25,6 +26,22 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)
ce->ce_flags &= ~CE_FSMONITOR_VALID;
}
+static int fsmonitor_hook_version(void)
+{
+ int hook_version;
+
+ if (git_config_get_int("core.fsmonitorhookversion", &hook_version))
+ return -1;
+
+ if (hook_version == HOOK_INTERFACE_VERSION1 ||
+ hook_version == HOOK_INTERFACE_VERSION2)
+ return hook_version;
+
+ warning("Invalid hook version '%i' in core.fsmonitorhookversion. "
+ "Must be 1 or 2.", hook_version);
+ return -1;
+}
+
int read_fsmonitor_extension(struct index_state *istate, const void *data,
unsigned long sz)
{
@@ -158,8 +175,8 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n
void refresh_fsmonitor(struct index_state *istate)
{
struct strbuf query_result = STRBUF_INIT;
- int query_success = 0;
- size_t bol; /* beginning of line */
+ int query_success = 0, hook_version = -1;
+ size_t bol = 0; /* beginning of line */
uint64_t last_update;
struct strbuf last_update_token = STRBUF_INIT;
char *buf;
@@ -167,6 +184,9 @@ void refresh_fsmonitor(struct index_state *istate)
if (!core_fsmonitor || istate->fsmonitor_has_run_once)
return;
+
+ hook_version = fsmonitor_hook_version();
+
istate->fsmonitor_has_run_once = 1;
trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");
@@ -175,27 +195,60 @@ void refresh_fsmonitor(struct index_state *istate)
* should be inclusive to ensure we don't miss potential changes.
*/
last_update = getnanotime();
- strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
+ if (hook_version == HOOK_INTERFACE_VERSION1)
+ strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
/*
- * If we have a last update time, call query_fsmonitor for the set of
- * changes since that time, else assume everything is possibly dirty
+ * If we have a last update token, call query_fsmonitor for the set of
+ * changes since that token, else assume everything is possibly dirty
* and check it all.
*/
if (istate->fsmonitor_last_update) {
- query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,
- istate->fsmonitor_last_update, &query_result);
+ if (hook_version == -1 || hook_version == HOOK_INTERFACE_VERSION2) {
+ query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION2,
+ istate->fsmonitor_last_update, &query_result);
+
+ if (query_success) {
+ if (hook_version < 0)
+ hook_version = HOOK_INTERFACE_VERSION2;
+
+ /*
+ * First entry will be the last update token
+ * Need to use a char * variable because static
+ * analysis was suggesting to use strbuf_addbuf
+ * but we don't want to copy the entire strbuf
+ * only the the chars up to the first NUL
+ */
+ buf = query_result.buf;
+ strbuf_addstr(&last_update_token, buf);
+ if (!last_update_token.len) {
+ warning("Empty last update token.");
+ query_success = 0;
+ } else {
+ bol = last_update_token.len + 1;
+ }
+ } else if (hook_version < 0) {
+ hook_version = HOOK_INTERFACE_VERSION1;
+ if (!last_update_token.len)
+ strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
+ }
+ }
+
+ if (hook_version == HOOK_INTERFACE_VERSION1) {
+ query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION1,
+ istate->fsmonitor_last_update, &query_result);
+ }
+
trace_performance_since(last_update, "fsmonitor process '%s'", core_fsmonitor);
trace_printf_key(&trace_fsmonitor, "fsmonitor process '%s' returned %s",
core_fsmonitor, query_success ? "success" : "failure");
}
/* a fsmonitor process can return '/' to indicate all entries are invalid */
- if (query_success && query_result.buf[0] != '/') {
+ if (query_success && query_result.buf[bol] != '/') {
/* Mark all entries returned by the monitor as dirty */
buf = query_result.buf;
- bol = 0;
- for (i = 0; i < query_result.len; i++) {
+ for (i = bol; i < query_result.len; i++) {
if (buf[i] != '\0')
continue;
fsmonitor_refresh_callback(istate, buf + bol);
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index cf0fda2d5a..fbfdcca000 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -32,11 +32,12 @@ write_integration_script () {
echo "$0: exactly 2 arguments expected"
exit 2
fi
- if test "$1" != 1
+ if test "$1" != 2
then
echo "Unsupported core.fsmonitor hook version." >&2
exit 1
fi
+ printf "last_update_token\0"
printf "untracked\0"
printf "dir1/untracked\0"
printf "dir2/untracked\0"
@@ -107,6 +108,7 @@ EOF
# test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' '
write_script .git/hooks/fsmonitor-test<<-\EOF &&
+ printf "last_update_token\0"
EOF
git update-index --fsmonitor &&
git update-index --fsmonitor-valid dir1/modified &&
@@ -167,6 +169,7 @@ EOF
# test that newly added files are marked valid
test_expect_success 'newly added files are marked valid' '
write_script .git/hooks/fsmonitor-test<<-\EOF &&
+ printf "last_update_token\0"
EOF
git add new &&
git add dir1/new &&
@@ -207,6 +210,7 @@ EOF
# test that *only* files returned by the integration script get flagged as invalid
test_expect_success '*only* files returned by the integration script get flagged as invalid' '
write_script .git/hooks/fsmonitor-test<<-\EOF &&
+ printf "last_update_token\0"
printf "dir1/modified\0"
EOF
clean_repo &&
@@ -276,6 +280,7 @@ do
# (if enabled) files unless it is told about them.
test_expect_success "status doesn't detect unreported modifications" '
write_script .git/hooks/fsmonitor-test<<-\EOF &&
+ printf "last_update_token\0"
:>marker
EOF
clean_repo &&
diff --git a/t/t7519/fsmonitor-all b/t/t7519/fsmonitor-all
index 691bc94dc2..94ab66bd3d 100755
--- a/t/t7519/fsmonitor-all
+++ b/t/t7519/fsmonitor-all
@@ -17,7 +17,6 @@ fi
if test "$1" != 1
then
- echo "Unsupported core.fsmonitor hook version." >&2
exit 1
fi
diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index d8e7a1e5ba..264b9daf83 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -26,8 +26,7 @@ if ($version == 1) {
# subtract one second to make sure watchman will return all changes
$time = int ($time / 1000000000) - 1;
} else {
- die "Unsupported query-fsmonitor hook version '$version'.\n" .
- "Falling back to scanning...\n";
+ exit 1;
}
my $git_work_tree;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/4] fsmonitor: change last update timestamp on the index_state to opaque token
From: Kevin Willford via GitGitGadget @ 2020-01-07 19:04 UTC (permalink / raw)
To: git; +Cc: Kevin Willford, Junio C Hamano, Kevin Willford
In-Reply-To: <pull.510.git.1578423871.gitgitgadget@gmail.com>
From: Kevin Willford <Kevin.Willford@microsoft.com>
Some file system monitors might not use or take a timestamp for processing
and in the case of watchman could have race conditions with using a
timestamp. Watchman uses something called a clockid that is used for race
free queries to it. The clockid for watchman is simply a string.
Change the fsmonitor_last_update from being a uint64_t to a char pointer
so that any arbitrary data can be stored in it and passed back to the
fsmonitor.
Signed-off-by: Kevin Willford <Kevin.Willford@microsoft.com>
---
cache.h | 2 +-
fsmonitor.c | 49 ++++++++++++++++++++++------------
t/helper/test-dump-fsmonitor.c | 2 +-
3 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/cache.h b/cache.h
index 1554488d66..170c125db3 100644
--- a/cache.h
+++ b/cache.h
@@ -324,7 +324,7 @@ struct index_state {
struct hashmap dir_hash;
struct object_id oid;
struct untracked_cache *untracked;
- uint64_t fsmonitor_last_update;
+ char *fsmonitor_last_update;
struct ewah_bitmap *fsmonitor_dirty;
struct mem_pool *ce_mem_pool;
struct progress *progress;
diff --git a/fsmonitor.c b/fsmonitor.c
index 868cca01e2..9860587225 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -6,8 +6,9 @@
#include "run-command.h"
#include "strbuf.h"
-#define INDEX_EXTENSION_VERSION (1)
-#define HOOK_INTERFACE_VERSION (1)
+#define INDEX_EXTENSION_VERSION1 (1)
+#define INDEX_EXTENSION_VERSION2 (2)
+#define HOOK_INTERFACE_VERSION (1)
struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
@@ -32,17 +33,26 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
uint32_t ewah_size;
struct ewah_bitmap *fsmonitor_dirty;
int ret;
+ uint64_t timestamp;
+ struct strbuf last_update = STRBUF_INIT;
- if (sz < sizeof(uint32_t) + sizeof(uint64_t) + sizeof(uint32_t))
+ if (sz < sizeof(uint32_t) + 1 + sizeof(uint32_t))
return error("corrupt fsmonitor extension (too short)");
hdr_version = get_be32(index);
index += sizeof(uint32_t);
- if (hdr_version != INDEX_EXTENSION_VERSION)
+ if (hdr_version == INDEX_EXTENSION_VERSION1) {
+ timestamp = get_be64(index);
+ strbuf_addf(&last_update, "%"PRIu64"", timestamp);
+ index += sizeof(uint64_t);
+ } else if (hdr_version == INDEX_EXTENSION_VERSION2) {
+ strbuf_addstr(&last_update, index);
+ index += last_update.len + 1;
+ } else {
return error("bad fsmonitor version %d", hdr_version);
+ }
- istate->fsmonitor_last_update = get_be64(index);
- index += sizeof(uint64_t);
+ istate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);
ewah_size = get_be32(index);
index += sizeof(uint32_t);
@@ -79,7 +89,6 @@ void fill_fsmonitor_bitmap(struct index_state *istate)
void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
{
uint32_t hdr_version;
- uint64_t tm;
uint32_t ewah_start;
uint32_t ewah_size = 0;
int fixup = 0;
@@ -89,11 +98,12 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
(uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
- put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
+ put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
strbuf_add(sb, &hdr_version, sizeof(uint32_t));
- put_be64(&tm, istate->fsmonitor_last_update);
- strbuf_add(sb, &tm, sizeof(uint64_t));
+ strbuf_addstr(sb, istate->fsmonitor_last_update);
+ strbuf_addch(sb, 0); /* Want to keep a NUL */
+
fixup = sb->len;
strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
@@ -110,9 +120,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
}
/*
- * Call the query-fsmonitor hook passing the time of the last saved results.
+ * Call the query-fsmonitor hook passing the last update token of the saved results.
*/
-static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
+static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)
{
struct child_process cp = CHILD_PROCESS_INIT;
@@ -121,7 +131,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
argv_array_push(&cp.args, core_fsmonitor);
argv_array_pushf(&cp.args, "%d", version);
- argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update);
+ argv_array_pushf(&cp.args, "%s", last_update);
cp.use_shell = 1;
cp.dir = get_git_work_tree();
@@ -151,6 +161,7 @@ void refresh_fsmonitor(struct index_state *istate)
int query_success = 0;
size_t bol; /* beginning of line */
uint64_t last_update;
+ struct strbuf last_update_token = STRBUF_INIT;
char *buf;
unsigned int i;
@@ -164,6 +175,7 @@ void refresh_fsmonitor(struct index_state *istate)
* should be inclusive to ensure we don't miss potential changes.
*/
last_update = getnanotime();
+ strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
/*
* If we have a last update time, call query_fsmonitor for the set of
@@ -217,18 +229,21 @@ void refresh_fsmonitor(struct index_state *istate)
}
strbuf_release(&query_result);
- /* Now that we've updated istate, save the last_update time */
- istate->fsmonitor_last_update = last_update;
+ /* Now that we've updated istate, save the last_update_token */
+ FREE_AND_NULL(istate->fsmonitor_last_update);
+ istate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);
}
void add_fsmonitor(struct index_state *istate)
{
unsigned int i;
+ struct strbuf last_update = STRBUF_INIT;
if (!istate->fsmonitor_last_update) {
trace_printf_key(&trace_fsmonitor, "add fsmonitor");
istate->cache_changed |= FSMONITOR_CHANGED;
- istate->fsmonitor_last_update = getnanotime();
+ strbuf_addf(&last_update, "%"PRIu64"", getnanotime());
+ istate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);
/* reset the fsmonitor state */
for (i = 0; i < istate->cache_nr; i++)
@@ -250,7 +265,7 @@ void remove_fsmonitor(struct index_state *istate)
if (istate->fsmonitor_last_update) {
trace_printf_key(&trace_fsmonitor, "remove fsmonitor");
istate->cache_changed |= FSMONITOR_CHANGED;
- istate->fsmonitor_last_update = 0;
+ FREE_AND_NULL(istate->fsmonitor_last_update);
}
}
diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index 2786f47088..975f0ac890 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -13,7 +13,7 @@ int cmd__dump_fsmonitor(int ac, const char **av)
printf("no fsmonitor\n");
return 0;
}
- printf("fsmonitor last update %"PRIuMAX"\n", (uintmax_t)istate->fsmonitor_last_update);
+ printf("fsmonitor last update %s\n", istate->fsmonitor_last_update);
for (i = 0; i < istate->cache_nr; i++)
printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? "+" : "-");
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/4] fsmonitor: start using an opaque token for last update
From: Kevin Willford via GitGitGadget @ 2020-01-07 19:04 UTC (permalink / raw)
To: git; +Cc: Kevin Willford, Junio C Hamano
When using watchman for the fsmonitor there are race conditions when using a
timestamp for the last update as noted in the watchman documentation "Using
a timestamp is prone to race conditions in understanding the complete state
of the file tree." https://facebook.github.io/watchman/docs/clockspec.html
Watchman uses something referred to as a clock id to remove these race
conditions. In order to use a clock id for the last update, an opaque value
needs to be returned by the fsmonitor hook and saved so that it can be
passed back to the fsmonitor hook for the next query. This requires a new
version for the fsmonitor index extension and new versions for the fsmonitor
watchman hooks. We also need to make sure version 1 of the hook continues to
work because users may not update their hook when they update git and we
want things to continue to work on repos still using a version 1 hook.
Kevin Willford (4):
fsmonitor: change last update timestamp on the index_state to opaque
token
fsmonitor: handle version 2 of the hooks that will use opaque token
fsmonitor: add fsmonitor hook scripts for version 2
fsmonitor: update documentation for hook version and watchman hooks
Documentation/config/core.txt | 11 ++
Documentation/githooks.txt | 13 +-
cache.h | 2 +-
fsmonitor.c | 120 ++++++++++----
t/helper/test-dump-fsmonitor.c | 2 +-
t/t7519-status-fsmonitor.sh | 7 +-
t/t7519/fsmonitor-all | 1 -
t/t7519/fsmonitor-all-v2 | 21 +++
t/t7519/fsmonitor-watchman | 3 +-
t/t7519/fsmonitor-watchman-v2 | 173 +++++++++++++++++++++
templates/hooks--fsmonitor-watchman.sample | 138 ++++++++++------
11 files changed, 405 insertions(+), 86 deletions(-)
create mode 100755 t/t7519/fsmonitor-all-v2
create mode 100755 t/t7519/fsmonitor-watchman-v2
base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-510%2Fkewillford%2Ffsmonitor_opaque_token-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-510/kewillford/fsmonitor_opaque_token-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/510
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 0/1] Update imap-send.c, fix incompatibilities with OpenSSL 1.1.x
From: Johannes Schindelin @ 2020-01-07 18:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Liam Huang via GitGitGadget, Liam Huang, git
In-Reply-To: <xmqqv9pn5hgl.fsf@gitster-ct.c.googlers.com>
Hi Junio,
On Tue, 7 Jan 2020, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Hi Liam,
> >
> > On Tue, 7 Jan 2020, Liam Huang via GitGitGadget wrote:
> >
> >> Some APIs have been changed since OpenSSL 1.1.0, so fix incompatibilities
> >> with OpenSSL 1.1.x.
> >
> > In your PR, the "Checks" tab shows that this breaks the build for all
> > non-32-bit Linux builds and for Windows. Here is an excerpt of the failed
> > `linux-clang` build:
> > -- snip --
> > ...
> > Could you fix those compile errors, please?
> >
> > While at it, please also fix your author email: it should match your
> > _real_ email address, i.e. "liamhuang0205@gmail.com", not
> > "Liam0205@users.noreply.github.com".
>
> Also, please do *not* CC iterations of a patch to me that hasn't
> seen a concensus that it is a good idea on the list yet, unless
> you know I am the area expert and am interested in seeing it.
I am afraid that I do not know of any means to teach GitGitGadget to make
that call whether it has seen a consensus.
And I fear that you are asking me to punt back that decision to
contributors, i.e. put a lot of the burden of knowing how Git
contributions are expected to progress _away_ from GitGitGadget. It is,
however, the explicit mission of GitGitGadget to _take that responsibility
of knowing all these things and not err at any step along the way *from*
the contributors_.
Of course, I can teach GitGitGadget to not Cc: you. Like, always. Not sure
that you would like that any better because you would not even be Cc:ed
once consensus was reached. So I'm not sure that I want to put in that
work for something you will equally hate in the end.
Or do you have any splendid ideas how this could be made easy on you _and_
on contributors (and for bonus points, _also_ on me)?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/3] graph: fix case that hit assert()
From: Derrick Stolee @ 2020-01-07 18:47 UTC (permalink / raw)
To: Jeff King, Derrick Stolee via GitGitGadget
Cc: git, brad, sunshine, Derrick Stolee, Junio C Hamano
In-Reply-To: <20200107153006.GA20591@coredump.intra.peff.net>
On 1/7/2020 10:30 AM, Jeff King wrote:
> On Tue, Jan 07, 2020 at 02:55:45PM +0000, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> A failure was reported in "git log --graph --all" with the new
>> graph-rendering logic. Create a test case that matches the
>> topology of that example and uses an explicit ref ordering instead
>> of the "--all" option. The test would fail with the following error:
>>
>> graph.c:1228: graph_output_collapsing_line: Assertion
>> `graph->mapping[i - 3] == target' failed.
>>
>> The situation is a little complicated, so let's break it down.
>
> First off, thanks for digging into this so promptly. Your solution looks
> correct to me. Everything else I'll mention here are nits. :)
>
> Your commit message starts off talking about the test, but without
> describing what's interesting about it. I think the answer is that we
> have two "skewed" merge parents for the same merge; maybe it would make
> sense to lead with that. I.e.:
>
> Subject: graph: drop assert() for merge with two collapsing parents
>
> When "git log --graph" shows a merge commit that has two collapsing
> lines, like:
>
> [your diagram]
>
> we trigger an assert():
>
> graph.c:1228: graph_output_collapsing_line: Assertion
> `graph->mapping[i - 3] == target' failed.
>
> ...and so on...
Good points.
>> The assert was introduced by eaf158f8 ("graph API: Use horizontal
>> lines for more compact graphs", 2009-04-21), which is quite old.
>> This assert is trying to say that when we complete a horizontal
>> line with a single slash, it is because we have reached our target.
>
> Thanks for this final sentence; writing that out in English made the
> purpose of the assert() much clearer.
>
> That could perhaps be an argument in favor of writing it as a BUG()
> with a similar human-readable explanation. I guess there was already a
> comment in the code, but it didn't quite click with me as much as what
> you wrote above.
>
>> It is actually the _second_ collapsing line that hits this assert.
>> The reason we are in this code path is because we are collapsing
>> the first line, and it in that case we are hitting our target now
>
> s/it//
>
>> that the horizontal line is complete. However, the second line
>> cannot be a horizontal line, so it will collapse without horizontal
>> lines. In this case, it is inappropriate to assert that we have
>> reached our target, as we need to continue for another column
>> before reaching the target. Dropping the assert is safe here.
>
> I think that makes sense. My big concern here is that the assert() was
> preventing us from looking out of bounds in the graph->mapping array,
> but I don't think that's the case here.
>
> Worth mentioning that this was due to 0f0f389f12 (graph: tidy up display
> of left-skewed merges, 2019-10-15), in case somebody has to later dig
> deeper?
Can do.
>> Second, the horizontal lines in that first line drop their coloring.
>> This is due to a use of graph_line_addch() instead of
>> graph_line_write_column(). Using a ternary operator to pick the
>> character is nice for compact code, but we actually need a column
>> to provide the color.
>
> It seems like this is a totally separate bug, and could be its own
> commit?
>
> I think it's also caused by 0f0f389f12, which actually introduced the
> seen_parent mechanism that you're correcting here.
You're right. These are better split. Any idea how to test the color?
(I'm pretty sure we have some tests for this... I can dig around.)
>> Helped-by: Jeff King <peff@peff.net>
>> Reported-by: Bradley Smith <brad@brad-smith.co.uk>
>
> I don't know that I did much, but OK. :)
>
> Thanks once again Bradley for the reproducible case.
>
>> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
>> index 18709a723e..ddf6f6f5d3 100755
>> --- a/t/t4215-log-skewed-merges.sh
>> +++ b/t/t4215-log-skewed-merges.sh
>> @@ -240,4 +240,46 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
>> EOF
>> '
>>
>> +test_expect_success 'log --graph with multiple tips' '
>
> This nicely covers the assert() problem. Could we check the same case
> with "--color" and test_decode_color to check the coloring issue (see
> t4214 for some prior art)?
Thanks for pointing me to existing color tests. I'll add one to my v2.
-Stolee
^ permalink raw reply
* Re: [PATCH 2/3] graph: replace assert() with graph_assert() macro
From: Derrick Stolee @ 2020-01-07 18:45 UTC (permalink / raw)
To: Eric Sunshine, Jeff King
Cc: Derrick Stolee via GitGitGadget, Git List, Bradley Smith,
Derrick Stolee, Junio C Hamano
In-Reply-To: <CAPig+cTu=iAeQNm8z53cyG8C1dgokpZBvRVgev091nBFg8tCXQ@mail.gmail.com>
On 1/7/2020 10:51 AM, Eric Sunshine wrote:
> On Tue, Jan 7, 2020 at 10:36 AM Jeff King <peff@peff.net> wrote:
>> On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote:
>>> The assert() macro is sometimes compiled out. Instead, switch these into
>>> BUG() statements using our own custom macro.
>>
>> I can buy the argument that compiling with and without NDEBUG can lead
>> to confusion. But if that is the case, wouldn't it be so for all of the
>> assert() calls, not just ones in the graph code?
>
> This wasn't just a matter of potential confusion. It's one thing to
> have assert()s in the code in general, but another thing when a
> scripted test specifically depends upon the asserted condition, as was
> the case with the test as originally proposed. Since the final patch
> series removes that particular assert() altogether, it's perhaps not
> that important anymore.
I'm happy to drop this commit, too. I misunderstood your point.
-Stolee
^ permalink raw reply
* Re: [PATCH 3/3] t4215: add bigger graph collapse test
From: Derrick Stolee @ 2020-01-07 18:44 UTC (permalink / raw)
To: Jeff King, Junio C Hamano
Cc: James Coglan, Derrick Stolee via GitGitGadget, git, brad,
sunshine, Derrick Stolee
In-Reply-To: <20200107180415.GB48806@coredump.intra.peff.net>
On 1/7/2020 1:04 PM, Jeff King wrote:
> On Tue, Jan 07, 2020 at 10:02:48AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:
>>>
>>>> From: Derrick Stolee <dstolee@microsoft.com>
>>>>
>>>> A previous test in t4215-log-skewed-merges.sh was added to demonstrate
>>>> exactly the topology of a reported failure in "git log --graph". While
>>>> investigating the fix, we realized that multiple edges that could
>>>> collapse with horizontal lines were not doing so.
>>>
>>> Thanks for constructing this larger case.
>>>
>>> As for including this patch, I could take or leave it for now. I like
>>> the idea of documenting things further, but unless it's marked
>>> expect_failure, I don't think it's going to call anybody's attention
>>> more than this thread already has.
I think that's fine. I do believe this example demonstrates that the
behavior has changed, so adding an expect_failure (with the correct
expected output) may provide more motivation to fix the issue.
>>> So I'd love to hear what James thinks should happen here, given that
>>> it's an extension of his other work. But I'd just as soon punt on the
>>> patch until we decide whether it _should_ change (and then either mark
>>> it with expect_failure, or include the test along with a patch changing
>>> the behavior).
>>
>> ... and nobody CC'ed the person from whom they want to hear opinion?
>
> Doh, thank you. :) I cc'd him in the earlier thread, but GGG creates a
> new one.
Sorry, that was my fault. I definitely intended to CC him, but forgot
when going through those who _wrote_ on the previous thread.
-Stolee
^ permalink raw reply
* Re: [PATCH 3/3] t4215: add bigger graph collapse test
From: Jeff King @ 2020-01-07 18:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: James Coglan, Derrick Stolee via GitGitGadget, git, brad,
sunshine, Derrick Stolee
In-Reply-To: <xmqqftgr5elz.fsf@gitster-ct.c.googlers.com>
On Tue, Jan 07, 2020 at 10:02:48AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:
> >
> >> From: Derrick Stolee <dstolee@microsoft.com>
> >>
> >> A previous test in t4215-log-skewed-merges.sh was added to demonstrate
> >> exactly the topology of a reported failure in "git log --graph". While
> >> investigating the fix, we realized that multiple edges that could
> >> collapse with horizontal lines were not doing so.
> >
> > Thanks for constructing this larger case.
> >
> > As for including this patch, I could take or leave it for now. I like
> > the idea of documenting things further, but unless it's marked
> > expect_failure, I don't think it's going to call anybody's attention
> > more than this thread already has.
> >
> > So I'd love to hear what James thinks should happen here, given that
> > it's an extension of his other work. But I'd just as soon punt on the
> > patch until we decide whether it _should_ change (and then either mark
> > it with expect_failure, or include the test along with a patch changing
> > the behavior).
>
> ... and nobody CC'ed the person from whom they want to hear opinion?
Doh, thank you. :) I cc'd him in the earlier thread, but GGG creates a
new one.
-Peff
^ permalink raw reply
* Re: Git bundle create produces inconsistent bundle
From: Jeff King @ 2020-01-07 18:03 UTC (permalink / raw)
To: Andre Loconte; +Cc: git@vger.kernel.org
In-Reply-To: <QBxpUR_NauPk0G8X2KKsqzlrfyxNuA4OoNR3Dm1KpHMNEELiSUxKr_IDM_qghObDt4aVv-bjg1ZQtCYRgxArdGsK52wCuO1LbsqzlHBto-k=@protonmail.com>
On Tue, Jan 07, 2020 at 05:44:47PM +0000, Andre Loconte wrote:
> I am running into an issue where "git bundle create" doesn't always
> produce the same bundle, provided the exact same arguments and the
> exact same repo.
This is expected. A bundle contains a packfile, and the packfile
generation is sensitive to several things:
- the delta search is threaded (to the same number of threads as you
have CPUs). Try "git -c pack.threads=1 bundle create ...".
- we try to reuse on-disk deltas when possible. So the exact set of
packs you have (or which objects are loose) may impact the result
slightly. So between two different clones of the same repository,
I'd expect different results.
Possibly repacking first with "git -c pack.threads=1 repack -adf" might
get you an identical on-disk state in two different clones (assuming you
have the same refs and reflogs in each!).
But as you can see, we don't in general expect packfiles to be
byte-stable (this is also the reason that git clones aren't easily
resumable).
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] t4215: add bigger graph collapse test
From: Junio C Hamano @ 2020-01-07 18:02 UTC (permalink / raw)
To: Jeff King, James Coglan
Cc: Derrick Stolee via GitGitGadget, git, brad, sunshine,
Derrick Stolee
In-Reply-To: <20200107153922.GC20591@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> A previous test in t4215-log-skewed-merges.sh was added to demonstrate
>> exactly the topology of a reported failure in "git log --graph". While
>> investigating the fix, we realized that multiple edges that could
>> collapse with horizontal lines were not doing so.
>
> Thanks for constructing this larger case.
>
> As for including this patch, I could take or leave it for now. I like
> the idea of documenting things further, but unless it's marked
> expect_failure, I don't think it's going to call anybody's attention
> more than this thread already has.
>
> So I'd love to hear what James thinks should happen here, given that
> it's an extension of his other work. But I'd just as soon punt on the
> patch until we decide whether it _should_ change (and then either mark
> it with expect_failure, or include the test along with a patch changing
> the behavior).
... and nobody CC'ed the person from whom they want to hear opinion?
;-)
^ permalink raw reply
* Git bundle create produces inconsistent bundle
From: Andre Loconte @ 2020-01-07 17:44 UTC (permalink / raw)
To: git@vger.kernel.org
Hello,
I am running into an issue where "git bundle create" doesn't always produce the same bundle, provided the exact same arguments and the exact same repo.
Where it gets tricky is that:
- I cannot reproduce the issue with random repos from Github (tried with ~10 repos and 30 bundles per repo);
- I cannot reproduce the issue with the same OS and git version on different hardware ;
- I can reproduce the issue on colleagues' workstation (environment is identical down to the hardware, see below).
Environment:
- GNU/Linux 5.3.0-24-generic Ubuntu 19.10 x86_64 ;
- git version 2.20.1 ;
- Intel Xeon Silver 4114 CPU @ 2.20GHz × 20 ;
- 4 × Hynix HMA81GR7CJR8N-VK 8GB DDR4-2666 ECC.
I have put together a bash PoC [1]. And there are some example output in the comments (consistent [2], inconsistent [3]).
[1] https://gist.github.com/alct/05cc9a2b4657d51669c96cb22cd5c4a6
[2] https://gist.github.com/alct/05cc9a2b4657d51669c96cb22cd5c4a6#gistcomment-3130206
[3] https://gist.github.com/alct/05cc9a2b4657d51669c96cb22cd5c4a6#gistcomment-3130213
I cannot share the affected repos but I can run any diagnostic tool against them.
Would you have any clue on what could produce such behavior?
Best,
A.L.
^ permalink raw reply
* SHA-1 chosen-prefix colission attack
From: Kevin Daudt @ 2020-01-07 17:31 UTC (permalink / raw)
To: git
Researchers published new advances in creating collisions in SHA-1
hashes: https://sha-mbles.github.io/
> As a side result, this shows that it now costs less than 100k USD to
> break cryptography with a security level of 64 bits (i.e. to compute
> 264 operations of symmetric cryptography).
Kevin
^ permalink raw reply
* Re: [PATCH 0/3] Fix two bugs in graph.c
From: Junio C Hamano @ 2020-01-07 17:13 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget; +Cc: git, peff, brad, sunshine, Derrick Stolee
In-Reply-To: <pull.517.git.1578408947.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This is a possible fix for the bug reported in [1].
>
> The first commit fixes the runtime failure due to the assert() statement.
>
> The second commit replaces the assert() statements with a macro that
> triggers a BUG().
>
> The third commit adds another test that shows a more complicated example and
> how the new code in v2.25.0-rc1 has a behavior change that is not
> necessarily wanted.
>
> Thanks, -Stolee
Thanks, all, for so quickly resolving the issue.
Will queue.
>
> [1]
> https://lore.kernel.org/git/CAHt=fUXTHc4JPsapvHKnw5vHhp2cBOYRNfdaSDWBUnKt8fWfeA@mail.gmail.com/
>
> Derrick Stolee (3):
> graph: fix case that hit assert()
> graph: replace assert() with graph_assert() macro
> t4215: add bigger graph collapse test
>
> graph.c | 39 +++++++------
> t/t4215-log-skewed-merges.sh | 105 +++++++++++++++++++++++++++++++++++
> 2 files changed, 126 insertions(+), 18 deletions(-)
>
>
> base-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-517%2Fderrickstolee%2Fgraph-assert-fix-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-517/derrickstolee/graph-assert-fix-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/517
^ permalink raw reply
* Re: [PATCH 1/1] Preserve topological ordering after merges This modifies the algorithm of topopological ordering. The problem with the current algoritm is that it displays the graph below as follows
From: Junio C Hamano @ 2020-01-07 17:01 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Sergey Rudyshin via GitGitGadget, git, Sergey Rudyshin
In-Reply-To: <nycvar.QRO.7.76.6.2001071305330.46@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Sergey,
>
> please note that the commit message's first line should not be longer than
> 76 characters per line, and it should be followed by an empty line. I
> think that you forgot the empty line, looking at
> https://github.com/gitgitgadget/git/pull/514/commits/542a02020c8578d0e004cb9c929bced8719b902a
Thanks for a good suggestion.
Also, please do *not* CC iterations of a patch to me that hasn't
seen a concensus that it is a good idea on the list yet, unless
you know I am the area expert and am interested in seeing it.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/1] Update imap-send.c, fix incompatibilities with OpenSSL 1.1.x
From: Junio C Hamano @ 2020-01-07 17:01 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Liam Huang via GitGitGadget, Liam Huang, git
In-Reply-To: <nycvar.QRO.7.76.6.2001071313580.46@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Liam,
>
> On Tue, 7 Jan 2020, Liam Huang via GitGitGadget wrote:
>
>> Some APIs have been changed since OpenSSL 1.1.0, so fix incompatibilities
>> with OpenSSL 1.1.x.
>
> In your PR, the "Checks" tab shows that this breaks the build for all
> non-32-bit Linux builds and for Windows. Here is an excerpt of the failed
> `linux-clang` build:
> -- snip --
> ...
> Could you fix those compile errors, please?
>
> While at it, please also fix your author email: it should match your
> _real_ email address, i.e. "liamhuang0205@gmail.com", not
> "Liam0205@users.noreply.github.com".
Also, please do *not* CC iterations of a patch to me that hasn't
seen a concensus that it is a good idea on the list yet, unless
you know I am the area expert and am interested in seeing it.
Thanks.
^ permalink raw reply
* Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH
From: Junio C Hamano @ 2020-01-07 16:58 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, git, Miriam R.
In-Reply-To: <20200107110145.GA1073219@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Jan 07, 2020 at 01:36:40AM +0000, brian m. carlson wrote:
>
>> In this function, we free the pointer we get from locate_in_PATH and
>> then check whether it's NULL. However, this is undefined behavior if
>> the pointer is non-NULL, since the C standard no longer permits us to
>> use a valid pointer after freeing it.
>>
>> The only case in which the C standard would permit this to be defined
>> behavior is if r were NULL, since it states that in such a case "no
>> action occurs" as a result of calling free.
>>
>> It's easy to suggest that this is not likely to be a problem, but we
>> know that GCC does aggressively exploit the fact that undefined
>> behavior can never occur to optimize and rewrite code, even when that's
>> contrary to the expectations of the programmer. It is, in fact, very
>> common for it to omit NULL pointer checks, just as we have here.
>
> OK, I agree it makes sense to be on the safe side here (and the patch is
> obviously the right fix).
>
>> Noticed-by: Miriam R. <mirucam@gmail.com>
>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>
> I think Miriam actually posted the same patch in her initial email:
>
> https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/
>
> I don't know how we want to handle authorship.
I think the explanation in the log message has as much value as, if
not more than, the actual patch text, in this case. Noticed-by: may
be striking the right balance.
Thanks, all.
^ permalink raw reply
* Re: [PATCH 0/5] refactor gpg-interface and add gpg verification for clones
From: Junio C Hamano @ 2020-01-07 16:54 UTC (permalink / raw)
To: Hans Jerry Illikainen; +Cc: git
In-Reply-To: <87lfqjnc5o.hji@dyntopia.com>
Hans Jerry Illikainen <hji@dyntopia.com> writes:
>> It almost makes me suspect that what you are trying to do with the
>> "verification upon cloning" may be much better done as a "verify the
>> tree for trustworthyness before checking it out to the working tree"
>> mechanism, where the trustworthyness of a tree-ish object may be
>> defined (and possibly made customizable by the policy of the project
>> the user is working on) like so:
>>
>> - A tree is trustworthy if it is the tree of a trustworthy commit.
>>
>> - A commit is trustworthy if
>>
>> . it carries a trusted signature, or
>> . it is pointed by a tag that carries a trusted signature, or
>> . it is reachable from a trustworthy commit.
>>
>> Now, it is prohibitively expensive to compute the trusttworthiness
>> of a tree, defined like the above, when checking it out, UNLESS you
>> force each and every commit to carry a trusted signature, which is
>> not necessarily practical in the real world.
>
> Even though you mention that it's computationally expensive, I kind of
> like this approach. It seems generalized enough that it doesn't need to
> be tied to a single operation like 'clone'.
Well, I don't ;-) But even if you are not signing each and every
commit, it may be possible that the reachability information kept in
the commit-graph may help reduce the cost of the computation. We'd
probably need a way to memoise which tags and commits have already
been verified earlier for trustworthiness for the scheme to work.
> Wouldn't that still forgo signature verification when doing something
> like:
>
> $ git fetch
> $ git checkout -b foo origin/branch-with-previously-unseen-commits
>
> unless either fetch or checkout is equipped with signature verification?
Yes, but I was assuming that "fetch", as an underlying mechanism for
"pull" would be what you'd teach the verification.
> Also, in case of a revoked key, this approach would require that tags
> signed with that key be deleted on origin.
I am not sure if I follow. An object that was signed back when a
key was OK and then the key gets revoked later---what should happen
to the trustworthiness of that object? Another object that was
obtained from elsewhere was verified to be trustworthy, but later we
found out that the key we thought was trusted was already revoked---
what should happen to the trustworthiness of that object?
In either case, I think it is an unrelated matter if the tag object
should be removed either here or at the origin. When spread *now*,
the key that was used to sign that tag object is known to have been
revoked, so while it is of no use to convey trustworthiness, if the
upstream project chooses to keep the tag (perhaps while re-issuing
a new version of the tag that points at the same object but signed
with a different key), that is perfectly fine, I would say.
> Maybe that's to be
> considered best practice anyway, but distro maintainers might not
> appreciate disappearing release tags.
I think that is best left as a policy decision for each project, and
we are not in the business of removing objects---we just should give
them tools to determine what key was used to sign an object, GPG or
whatever signing and key management infrastructure the projects
chooses to use gives them tools to decide if the key is what the
project trusts, and we give them tools to remove tags the project
finds unwanted. Combining the tools according to the policy decision
the project makes is outside the scope of what we do here.
^ permalink raw reply
* Re: [PATCH v2 1/1] unpack-trees: exit check_updates() early if updates are not wanted
From: Junio C Hamano @ 2020-01-07 16:37 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <dd277273324eaba5e1f4a368bf2e4d046033c776.1578380277.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> check_updates() has a lot of code that repeatedly checks whether
> o->update or o->dry_run are set. (Note that o->dry_run is a
> near-synonym for !o->update, but not quite as per commit 2c9078d05bf2
> ("unpack-trees: add the dry_run flag to unpack_trees_options",
> 2011-05-25).) In fact, this function almost turns into a no-op whenever
> the condition
> !o->update || o->dry_run
> is met. Simplify the code by checking this condition at the beginning
> of the function, and when it is true, do the few things that are
> relevant and return early.
Thanks; will queue.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox