* Re: [PATCH v2 1/2] Change how HTTP response body is returned
From: Jeff King @ 2019-01-04 10:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Masaya Suzuki, git, jrnieder, sunshine
In-Reply-To: <xmqqtviph835.fsf@gitster-ct.c.googlers.com>
On Thu, Jan 03, 2019 at 11:09:02AM -0800, Junio C Hamano wrote:
> > + if (dest->file) {
> > + /*
> > + * At this point, the file contains the response body of the
> > + * previous request. We need to truncate the file.
> > + */
> > + FILE *new_file = freopen(dest->filename, "w", dest->file);
>
> Now freopen() lets us restart the file anew with a new "FILE *".
>
> > + if (new_file == NULL) {
> > + error("Unable to open local file %s", dest->filename);
>
> error_errno(), perhaps?
>
> At this point, I presume that dest->file is closed by the failed
> freopen(), but dest->file is still non-NULL and causes further calls
> to http_request() with this dest would be a disaster? As long as
> the caller of this function reacts to HTTP_ERROR and kill the dest,
> it would be fine.
I also wondered what timing guarantees freopen() gives us (i.e., is it
possible for it to open and truncate the file, and then close the old
handle, flushing some in-memory buffer). C99 says:
The freopen function first attempts to close any file that is
associated with the specified stream. Failure to close the file is
ignored. The error and end-of-file indicators for the stream are
cleared.
So I think the order is OK for my concern, but not for yours. I.e., on
an error, dest->file is now undefined.
It might be nice to set "dest->file == NULL" in that case. There's no
guarantee that the caller did not hold onto its own copy of the handle,
but since this struct is only exposed internally within http.c, that's
probably OK.
The most robust thing would perhaps be:
fflush(dest->file);
ftruncate(fileno(dest->file), 0);
which leaves the handle intact.
(I agree with the rest of your review, especially that it would be
easier to read if this were split into separate refactor and
change-behavior steps).
-Peff
^ permalink raw reply
* Re: [PATCH v2] test-lib: check Bash version for '-x' without using shell arrays
From: SZEDER Gábor @ 2019-01-04 9:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Max Kirillov, Carlo Arenas, Jeff King,
Eric Sunshine, git
In-Reply-To: <xmqqa7khh4cw.fsf@gitster-ct.c.googlers.com>
On Thu, Jan 03, 2019 at 12:29:35PM -0800, Junio C Hamano wrote:
> Let's treat this as an independent and more urgent fix-up. I think
> it is sufficient to apply it to 2.20.x track, even though we could
> go back to 2.17.x and above.
>
> And then let's tentatively kick the "stress test" series out of
> 'pu', and have that series rebuilt on top of 'master' and this
> patch.
I rebased my '--stress' patch series on top of
'sg/test-bash-version-fix', and the result is the same as what's at
the tip of 'sg/stress-test' at 1d1416a34b.
^ permalink raw reply
* Re: git clone on page 44 of progit-en.1084.pdf (2nd try)
From: Jeff King @ 2019-01-04 8:45 UTC (permalink / raw)
To: Charles Kerr; +Cc: git
In-Reply-To: <CABMGpwQ5psB0DGJkWSbczAj+cTKXdsJ+cR6YbQhJpZMQZOHNrQ@mail.gmail.com>
On Thu, Jan 03, 2019 at 04:43:32PM -0800, Charles Kerr wrote:
> I am an easily-derailed newbie reading chapter 2 of progit-en.1084.pdf
Sort of orthogonal to your question, but I think this is a slightly
older version of the book. Note that it's constantly undergoing
revisions, and the latest version can always be gotten from:
https://github.com/progit/progit2/releases
(or https://git-scm.com/book, which is updated daily from that
repository).
The content in question is on page 26 in the most recent version, or
here in the web version:
https://git-scm.com/book/en/v2/Git-Basics-Getting-a-Git-Repository
Now, on to your actual questions...
> (1) On page 44 of my copy, where it first discusses git clone, there's
> reference to a "linkable library". How does this relate to
> "repository". I thought the aim here was to get a clone of a remote
> repository....
That is the aim. They just needed a sample repository to talk about, so
they used the libgit2 repository as an example. And that repository
holds code for a linkable library, but what it holds is not really
important for the example.
I think it would be more clear if the example used some name that was
more obviously an example repository (or even say, the repository for
git itself, which is easy to explain).
> (2) In each of the two example git-clone commands, the "libgit2" appears twice:
>
> git clone https://github.com/libgit2/libgit2
Right. On GitHub, there is a "libgit2" organization (for all work around
that library), which in turn has a repository named "libgit2" with the
actual library code (other repositories are things like bindings, etc).
But the "user/repo" convention is a GitHub thing (that is also used by
other hosting sites). Git only cares about what's in the final part of
the URL (and even then, only to choose a default directory name for the
local clone).
> What did make sense to me and worked:
>
> $ git clone https://github.com/komwomak/shtoza SNOW
>
> My GitHub user name: komwomak
> The name of the remote repository out on my GitHub stuff: shtoza
> The name of the local repository on the PC: SNOW
>
> I would appreciate knowing whether I have gotten the wrong end of some stick.
I think you understand it perfectly. If you're interested in suggesting
an improvement to the book, you can open an issue or a pull request at
the progit/progit2 repository I linked above.
-Peff
^ permalink raw reply
* Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.
From: Jeff King @ 2019-01-04 8:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Erin Dahlgren, git, Johannes Schindelin
In-Reply-To: <xmqqy381haup.fsf@gitster-ct.c.googlers.com>
On Thu, Jan 03, 2019 at 10:09:18AM -0800, Junio C Hamano wrote:
> >> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >> * the user has set GIT_DIR. It may be beneficial to disallow bogus
> >> * GIT_DIR values at some point in the future.
> >> */
> >> - if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {
> >> + if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE
> >> + startup_info->have_repository ||
> >> + // GIT_DIR_EXPLICIT
> >> + getenv(GIT_DIR_ENVIRONMENT)) {
> >
> > Same "//" style issue as above. I'm not sure how much value there is in
> > repeating the GIT_DIR_* cases here, as they're likely to grow out of
> > sync with the switch() statement above.
>
> It is unclear to me if the original code is doing the right thing
> under one condition, and this patch does not seem to change the
> behaviour.
>
> What happens if GIT_DIR environment is set to an invalid path and
> nongit_ok is non-NULL? setup_explicit_git_dir() would have assigned
> 1 to *nongit_ok, so have_repository is false at this point.
>
> We enter the if() statement in such a case, and end up calling
> setup_git_env(gitdir) using the bogus path that is not pointing at a
> repository. We leave have_repository to be false but the paths
> recorded in the_repository by setup_git_env() would all point at
> bogus places.
Yeah, that matches my analysis of what the code is doing.
Looks like this code (and comment) come from 73f192c991 (setup: don't
perform lazy initialization of repository state, 2017-06-20). I'm
guessing this was mostly a hack to quiet some test that complained about
the other changes in that commit. And indeed, dropping the getenv() half
of the conditional and running the tests in 73f192c991 gets me a failure
in t1050, which does:
GIT_DIR=non-existent git index-pack --strict --verify foo/.git/objects/pack/*.pack
But that's a bug in index-pack, which should be able to verify a pack
outside of a repository. And I think (according to bisection) we fixed
that in 14a9bd2898 (prepare_commit_graft: treat non-repository as a
noop, 2018-05-31), and the tests currently pass even with this patch
applied:
diff --git a/setup.c b/setup.c
index 1be5037f12..e2c03e9bbc 100644
--- a/setup.c
+++ b/setup.c
@@ -1132,7 +1132,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
* the user has set GIT_DIR. It may be beneficial to disallow bogus
* GIT_DIR values at some point in the future.
*/
- if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {
+ if (startup_info->have_repository) {
if (!the_repository->gitdir) {
const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
if (!gitdir)
> > At first I thought this could all be folded into the "else" clause of
> > the conditional above (which would make the logic much easier to
> > follow), but that wouldn't cover the case of GIT_DIR=/bogus, which is
> > what that getenv() is trying to handle here.
>
> Yes, but should GIT_DIR=/bogus even be touching the_repository?
Probably not. And from my poking around above, I think we're probably
safe to remove this hackery now.
> It is a separate clean-up and does not affect the validity of this
> simplification patchd, so I agreee with ...
>
> > So I think this is the best we can do for now.
Yep, it's definitely orthogonal. But if we do this cleanup as part of
it, we should be able to simplify further on top of Erin's patch, like
this:
diff --git a/setup.c b/setup.c
index eb8332bc02..edf65c44bf 100644
--- a/setup.c
+++ b/setup.c
@@ -1125,35 +1125,26 @@ const char *setup_git_directory_gently(int *nongit_ok)
// !nongit_ok || !*nongit_ok
startup_info->have_repository = 1;
startup_info->prefix = prefix;
+
if (prefix)
setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
else
setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
- }
- /*
- * Not all paths through the setup code will call 'set_git_dir()' (which
- * directly sets up the environment) so in order to guarantee that the
- * environment is in a consistent state after setup, explicitly setup
- * the environment if we have a repository.
- *
- * NEEDSWORK: currently we allow bogus GIT_DIR values to be set in some
- * code paths so we also need to explicitly setup the environment if
- * the user has set GIT_DIR. It may be beneficial to disallow bogus
- * GIT_DIR values at some point in the future.
- */
- if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE
- startup_info->have_repository ||
- // GIT_DIR_EXPLICIT
- getenv(GIT_DIR_ENVIRONMENT)) {
+ /*
+ * Not all paths through the setup code will call 'set_git_dir()' (which
+ * directly sets up the environment) so in order to guarantee that the
+ * environment is in a consistent state after setup, explicitly setup
+ * the environment if we have a repository.
+ */
if (!the_repository->gitdir) {
const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
if (!gitdir)
gitdir = DEFAULT_GIT_DIR_ENVIRONMENT;
setup_git_env(gitdir);
}
- if (startup_info->have_repository)
- repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
+
+ repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
}
strbuf_release(&dir);
I actually wonder if the "not all paths..." bit is even still true these
days (and if it is, I think we should consider fixing those code paths).
Certainly that setup_git_env() needed to trigger for the GIT_DIR=bogus,
but with that gone, any $GIT_DIR should have triggered GIT_DIR_EXPLICIT,
and any "default to .git" should be handled as part of discovery, I'd
think.
So what next? Erin, are you interested in using the details of this
conversation to take the cleanups a bit further? If not, then I can try
to prepare a few patches on top of yours.
-Peff
^ permalink raw reply related
* Re: [PATCH 1/3] Add 'human' date format
From: Jeff King @ 2019-01-04 7:50 UTC (permalink / raw)
To: Stephen P. Smith
Cc: git, Linus Torvalds, Junio C Hamano,
Ævar Arnfjörð Bjarmason
In-Reply-To: <2637944.PqRbksyA0G@thunderbird>
On Thu, Jan 03, 2019 at 06:19:56AM -0700, Stephen P. Smith wrote:
> On Thursday, January 3, 2019 12:37:35 AM MST Jeff King wrote:
> > I like the idea of "human", and I like the idea of "auto", but it seems
> > to me that these are really two orthogonal things. E.g., might some
> > people not want to do something like:
> >
> > git config log.date auto:relative
> I didn't see anything in the code which would prohibit setting something like
> that.
Yeah, I don't think supporting that is too hard. I was thinking
something like this:
diff --git a/date.c b/date.c
index 4486c028ac..f731803872 100644
--- a/date.c
+++ b/date.c
@@ -883,11 +883,6 @@ int parse_date(const char *date, struct strbuf *result)
return 0;
}
-static int auto_date_style(void)
-{
- return (isatty(1) || pager_in_use()) ? DATE_HUMAN : DATE_NORMAL;
-}
-
static enum date_mode_type parse_date_type(const char *format, const char **end)
{
if (skip_prefix(format, "relative", end))
@@ -907,8 +902,6 @@ static enum date_mode_type parse_date_type(const char *format, const char **end)
return DATE_NORMAL;
if (skip_prefix(format, "human", end))
return DATE_HUMAN;
- if (skip_prefix(format, "auto", end))
- return auto_date_style();
if (skip_prefix(format, "raw", end))
return DATE_RAW;
if (skip_prefix(format, "unix", end))
@@ -923,6 +916,14 @@ void parse_date_format(const char *format, struct date_mode *mode)
{
const char *p;
+ /* "auto:foo" is "if tty/pager, then foo, otherwise normal" */
+ if (skip_prefix(format, "auto:", &p)) {
+ if (isatty(1) || pager_in_use())
+ format = p;
+ else
+ format = "default";
+ }
+
/* historical alias */
if (!strcmp(format, "local"))
format = "default-local";
That removes "auto" completely. We could still support it as an alias
for "auto:human" with something like:
if (!strcmp(format, "auto"))
format = "auto:human";
but IMHO it is a simpler interface to just have the user be explicit
(this is meant to be set once in config, after all).
> > I don't personally care about using this myself, but we already had to
> > deal with retrofitting "local" as a modifier. I'd prefer to avoid making
> > the same mistake again.
> Since I wasn't involved could you summarize the you are referring to?
The format "local" was a variant of "default" that would use the local
timezone instead of the author's. But there was no way to format, say,
iso8601 in the local timezone. So we had to invent a new syntax that was
compatible ("iso8601-local"), and keep "local" around forever for
backwards compatibility. Not the end of the world, but we can avoid it
in this case with a little preparation.
> > (I'd actually argue that "log.date" should basically _always_ have the
> > "auto" behavior, since it tends to get treated as plumbing anyway, and I
> > suspect that anybody who sets log.date now would see subtle breakage
> > from scripts. But maybe it's too late at this point?).
> If auto isn't added to the "log.date" file, then the date behaviour is not
> changed from is currently in the code base. Therefore, there shouldn't be
> any breakage.
Right, this isn't a problem with your patches. I mean that the existing
"log.date" is arguably mis-designed, and we ought to have had something
like "auto" from day one (or even made it the default for log.date).
-Peff
^ permalink raw reply related
* Re: git-send-email warnings & process dying of signal 11
From: Carlo Arenas @ 2019-01-04 6:25 UTC (permalink / raw)
To: Rafał Miłecki; +Cc: git
In-Reply-To: <19f3c27ed059f5556131b3e0d2faafce@milecki.pl>
one important point to consider when tracking this bug is that perl is
not crashing, but is told to abort by some instrumentation (most
likely gcc's fortify) and as shown by :
*** buffer overflow detected ***: /usr/bin/perl terminated
Carlo
^ permalink raw reply
* Re: git version 2.20.1.windows.1 throws file name too long in gitk
From: Bret Barkelew @ 2019-01-04 3:38 UTC (permalink / raw)
To: Bryan Turner; +Cc: git
In-Reply-To: <CAGyf7-HcncFOfmm5McEkOvCTkHNdePDSEQDZwEAK=z1bmC=QKw@mail.gmail.com>
They are the exact same path (with a different drive letter).
Another thing I've been able to confirm is I uninstalled git 2.20.1
and installed 2.19.0 on the failing system, and the older version of
Git works.
I've also tried a suggested fix I saw elsewhere to enable long path
names in the Windows registry, which did not resolve the issue with
git 2.20.1.
Happy to collect any additional data.
- Bret
On Thu, Jan 3, 2019 at 7:10 PM Bryan Turner <bturner@atlassian.com> wrote:
>
> On Thu, Jan 3, 2019 at 6:21 PM Bret Barkelew <bret@corthon.com> wrote:
>>
>> When I open gitk in a particular repository under this version of Git
>> for Windows, I now get a dialog box that says "Error: couldn't execute
>> "git": file name too long". I've noticed that the most pronounced
>> effect is that I cannot see the file diffs (or sometimes the file name
>> list) for any of the commits. I don't know the exact repro, but I've
>> narrowed down the following things:
>>
>> - This does not happen with the same repo if I use a system that has
>> git 2.19.0.windows.1 on another machine.
>> - This does not happen on my current machine in a freshly cloned repo.
>
>
> How “deep” are the paths to the different clones on the different systems? Are all of the clones at exactly the same path on disk?
>
> Git on Windows is (by defaulted) limited by MAX_PATH, which is 260 characters. That length is calculated including the path to the repository itself and then to the file inside the repository. That means, for example, a given repository cloned to C:\repo may not have issues, but the same repository cloned to C:\Users\Bryan\Documents\workspaces\repo may.
>
>>
>> However, as soon as the remote updates with any changes on a given
>> fetch/pull, the repo is put in a bad state permanently.
>>
>> I've pasted the output from gitk below...
>>
>> couldn't execute "git": file name too long
>> couldn't execute "git": file name too long
>> while executing
>> "open $cmd r"
>> (procedure "getallcommits" line 48)
>> invoked from within
>> "getallcommits"
>> (procedure "readcache" line 80)
>> invoked from within
>> "readcache file827e200"
>> ("eval" body line 1)
>> invoked from within
>> "eval $script"
>> (procedure "dorunq" line 11)
>> invoked from within
>> "dorunq"
>> ("after" script)
>>
>> Happy to gather whatever data needed.
>> Thanks!
>> - Bret Barkelew
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: brian m. carlson @ 2019-01-04 2:57 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Jeff King, Duy Nguyen
In-Reply-To: <20190104021855.GB97188@google.com>
[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]
On Thu, Jan 03, 2019 at 06:18:55PM -0800, Jonathan Nieder wrote:
> I may be a minority in this opinion, but I had trouble understanding
> what --literally would do from its name. I suspect we can come up
> with a better name.
>
> Unfortunately, I'm terrible at coming up with names. :-P
>
> --dereference would be a good name when it comes to symlinks, but
> it's not a good name for reading what is on the other side of a pipe.
> On the plus side, it matches "diff" and "cp"'s name for the "follow
> symbolic links" option.
>
> --plain captures the desire a little better --- we want a plain
> read(2) from the file instead of trying to be smart and look at
> whether it is e.g. a block device. But in the context of "diff", that
> would seem more like an option that affects the output.
>
> What would you think of
>
> - always reading from fifos instead of describing them, since I've
> never encountered a use case where people want the latter
>
> - --dereference to control whether to follow symlinks
This is actually surprisingly difficult. The reason I implemented this
only for no-index mode is because there are actually several places we
can stat a file in the diff code, and implementing a --dereference
option that catches all of those cases and getting the option passed
down to them is non-trivial.
Since my primary environment is on Linux, I need to implement functional
--dereference semantics before implementing the FIFO functionality to
know if it works in my use case.
I agree that would be a nicer approach overall, so let me see if I can
take another stab at it in a way that isn't excessively complex. It
would also make the name less controversial, since "--dereference" is
the obvious choice for dereferencing symlinks.
> [...]
> > --- a/diff-no-index.c
> > +++ b/diff-no-index.c
> > @@ -75,7 +75,25 @@ static int populate_from_stdin(struct diff_filespec *s)
> > return 0;
> > }
> >
> > -static struct diff_filespec *noindex_filespec(const char *name, int mode)
> > +static int populate_literally(struct diff_filespec *s)
> > +{
> > + struct strbuf buf = STRBUF_INIT;
> > + size_t size = 0;
> > + int fd = xopen(s->path, O_RDONLY);
> > +
> > + if (strbuf_read(&buf, fd, 0) < 0)
> > + return error_errno("error while reading from '%s'", s->path);
> > +
> > + s->should_munmap = 0;
> > + s->data = strbuf_detach(&buf, &size);
> > + s->size = size;
> > + s->should_free = 1;
> > + s->read_literally = 1;
>
> Oh! --read-literally works perfectly for me as a name. :)
If I can't manage to split this out into two pieces, I'll pick that as
the name for the reroll.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* git version 2.20.1.windows.1 throws file name too long in gitk
From: Bret Barkelew @ 2019-01-04 2:21 UTC (permalink / raw)
To: git
When I open gitk in a particular repository under this version of Git
for Windows, I now get a dialog box that says "Error: couldn't execute
"git": file name too long". I've noticed that the most pronounced
effect is that I cannot see the file diffs (or sometimes the file name
list) for any of the commits. I don't know the exact repro, but I've
narrowed down the following things:
- This does not happen with the same repo if I use a system that has
git 2.19.0.windows.1 on another machine.
- This does not happen on my current machine in a freshly cloned repo.
However, as soon as the remote updates with any changes on a given
fetch/pull, the repo is put in a bad state permanently.
I've pasted the output from gitk below...
couldn't execute "git": file name too long
couldn't execute "git": file name too long
while executing
"open $cmd r"
(procedure "getallcommits" line 48)
invoked from within
"getallcommits"
(procedure "readcache" line 80)
invoked from within
"readcache file827e200"
("eval" body line 1)
invoked from within
"eval $script"
(procedure "dorunq" line 11)
invoked from within
"dorunq"
("after" script)
Happy to gather whatever data needed.
Thanks!
- Bret Barkelew
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: Jonathan Nieder @ 2019-01-04 2:18 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Jeff King, Duy Nguyen
In-Reply-To: <20181220002610.43832-1-sandals@crustytoothpaste.net>
Hi,
brian m. carlson wrote:
> In some shells, such as bash and zsh, it's possible to use a command
> substitution to provide the output of a command as a file argument to
> another process, like so:
>
> diff -u <(printf "a\nb\n") <(printf "a\nc\n")
>
> However, this syntax does not produce useful results with git diff
> --no-index.
Thanks much for fixing this. It's something I've run into, too.
[...]
> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -111,6 +111,11 @@ include::diff-options.txt[]
> "Unmerged". Can be used only when comparing the working tree
> with the index.
>
> +--literally::
> + Read the specified files literally, as `diff` would,
> + dereferencing any symlinks and reading data from pipes.
> + This option only works with `--no-index`.
I may be a minority in this opinion, but I had trouble understanding
what --literally would do from its name. I suspect we can come up
with a better name.
Unfortunately, I'm terrible at coming up with names. :-P
--dereference would be a good name when it comes to symlinks, but
it's not a good name for reading what is on the other side of a pipe.
On the plus side, it matches "diff" and "cp"'s name for the "follow
symbolic links" option.
--plain captures the desire a little better --- we want a plain
read(2) from the file instead of trying to be smart and look at
whether it is e.g. a block device. But in the context of "diff", that
would seem more like an option that affects the output.
What would you think of
- always reading from fifos instead of describing them, since I've
never encountered a use case where people want the latter
- --dereference to control whether to follow symlinks
?
[...]
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -75,7 +75,25 @@ static int populate_from_stdin(struct diff_filespec *s)
> return 0;
> }
>
> -static struct diff_filespec *noindex_filespec(const char *name, int mode)
> +static int populate_literally(struct diff_filespec *s)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + size_t size = 0;
> + int fd = xopen(s->path, O_RDONLY);
> +
> + if (strbuf_read(&buf, fd, 0) < 0)
> + return error_errno("error while reading from '%s'", s->path);
> +
> + s->should_munmap = 0;
> + s->data = strbuf_detach(&buf, &size);
> + s->size = size;
> + s->should_free = 1;
> + s->read_literally = 1;
Oh! --read-literally works perfectly for me as a name. :)
Jonathan
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: brian m. carlson @ 2019-01-04 2:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Duy Nguyen
In-Reply-To: <xmqqr2dukhw2.fsf@gitster-ct.c.googlers.com>
[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]
On Wed, Jan 02, 2019 at 10:56:45AM -0800, Junio C Hamano wrote:
> This is good as a goal, but the implementation seems to be overly
> eager to dereference any symlink or non-regular file found in any
> level of recursion. The use case presented as the justification in
> the proposed log message, and the explanation in the documentation,
> suggests that only the paths given from the command line are treated
> this way.
>
> It may make sense to do this as two orthgonal flags. Dereference
> symlinks and named pipes given from the command line is one use
> case. Dereference any symlinks encountered during recursion is
> another. And the latter might be useful even inside a repository
> as an option, even though the former would never make sense unless
> running in --no-index mode.
Yeah, I definitely think I'll be doing a reroll to address the recursion
issue as Peff suggested. I've just been on vacation and will be
traveling again soon, so I haven't gotten to it yet. Feel free to wait
for the reroll before picking it up.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* git clone on page 44 of progit-en.1084.pdf (2nd try)
From: Charles Kerr @ 2019-01-04 0:43 UTC (permalink / raw)
To: git
Hello-
I am an easily-derailed newbie reading chapter 2 of progit-en.1084.pdf
(1) On page 44 of my copy, where it first discusses git clone, there's
reference to a "linkable library". How does this relate to
"repository". I thought the aim here was to get a clone of a remote
repository....
(2) In each of the two example git-clone commands, the "libgit2" appears twice:
git clone https://github.com/libgit2/libgit2
What did make sense to me and worked:
$ git clone https://github.com/komwomak/shtoza SNOW
My GitHub user name: komwomak
The name of the remote repository out on my GitHub stuff: shtoza
The name of the local repository on the PC: SNOW
I would appreciate knowing whether I have gotten the wrong end of some stick.
crk
--
Charles R Kerr
8508 N Pamela St
Spokane WA
99208-9656
^ permalink raw reply
* Re: [PATCH v2 0/2] Accept error packets in any context
From: Masaya Suzuki @ 2019-01-04 0:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Jeff King, Josh Steadmon
In-Reply-To: <xmqqef9tfikg.fsf@gitster-ct.c.googlers.com>
On Thu, Jan 3, 2019 at 3:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Masaya Suzuki <masayasuzuki@google.com> writes:
>
> > This makes it possible for servers to send an error message back to clients in
> > an arbitrary situation.
> >
> > The first patch was originally sent in [1]. This version includes some fix.
> >
> > The second patch was originally sent in [2]. Later, this was cherry-picked in
> > [3]. In the discussion in [3], we agreed that this error packet handling should
> > be done only against the Git pack protocol handling code. With this agreement,
> > the patch series sent in [3] is abandoned (according to [4]). This is a patch
> > series based on that agreement on limiting the error packet handling.
>
> In short, are you shooting js/smart-http-detect-remote-error topic
> down and replacing it with this one?
>
> As that topic is not yet in 'next', I am perfectly fine doing that.
> I just want to make sure that is what you meant, as my reading of
> [4] was a bit fuzzy.
Sorry, I think I referenced a wrong email. [4] was meant to be
https://public-inbox.org/git/20181219233005.GI37614@google.com/. I
think he wants to have
https://public-inbox.org/git/20181116084725.GA31603@sigill.intra.peff.net/,
https://public-inbox.org/git/20181116084838.GB31603@sigill.intra.peff.net/,
and https://public-inbox.org/git/20181116084951.GC31603@sigill.intra.peff.net/
for js/smart-http-detect-remote-error.
^ permalink raw reply
* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
From: Stephen P. Smith @ 2019-01-03 23:57 UTC (permalink / raw)
To: Junio C Hamano, Philip Oakley
Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqq1s5th0u4.fsf@gitster-ct.c.googlers.com>
On Thursday, January 3, 2019 2:45:39 PM MST Junio C Hamano wrote:
> Philip Oakley <philipoakley@iee.org> writes:
> >>
> >> check_human_date 432000 "$THIS_YEAR_REGEX" # 5 days ago
> >
> > Just a quick bikeshed: if used, would this have a year end 5 day
> > roll-over error potential, or will it always use the single date?
>
> Hmph, interesting point. Indeed, date.c::show_date_normal() decides
> to hide the year portion if the timestamp and the current time share
> the same year, so on Thu Jan 3rd, an attempt to show a commit made
> on Mon Dec 31st of the same week would end up showing the year, so
> yes, I agree with you that the above would break.
>
Thanks Philip.
I wrote the test just before the new year so didn't see the rollover. I
haven't run the test this year.
sps
^ permalink raw reply
* Re: [PATCH v12 00/26] Convert "git stash" to C builtin
From: Junio C Hamano @ 2019-01-03 23:39 UTC (permalink / raw)
To: Paul-Sebastian Ungureanu; +Cc: git, t.gummerer, Johannes.Schindelin
In-Reply-To: <cover.1545331726.git.ungureanupaulsebastian@gmail.com>
Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:
> This is a new iteration of git-stash which also takes
> sd/stash-wo-user-name into account. I cherry-picked
> some of dscho's commits (from [1]) to keep the scripted
> version of `git stash` as `git-legacy-stash`.
I took a brief look and left a comment on 04/26 last year. I had
some time blocked for this topic today to take another look at the
whole series again. Thanks for working on this.
It seems that the last three or so steps are new, relative to the
previous round. I made sure that what is added back at step 24
exactly matches the result of merging sd/stash-wo-user-name into the
current 'master', but such a manual validation is error prone. Is
it possible to avoid "remove the scripted one prematurely at step
23, and then add it back as 'oops, that was wrong' fix at step 24"?
That would have been much more robust approach.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/2] Accept error packets in any context
From: Junio C Hamano @ 2019-01-03 23:05 UTC (permalink / raw)
To: Masaya Suzuki; +Cc: git, peff, steadmon
In-Reply-To: <20181229211915.161686-1-masayasuzuki@google.com>
Masaya Suzuki <masayasuzuki@google.com> writes:
> This makes it possible for servers to send an error message back to clients in
> an arbitrary situation.
>
> The first patch was originally sent in [1]. This version includes some fix.
>
> The second patch was originally sent in [2]. Later, this was cherry-picked in
> [3]. In the discussion in [3], we agreed that this error packet handling should
> be done only against the Git pack protocol handling code. With this agreement,
> the patch series sent in [3] is abandoned (according to [4]). This is a patch
> series based on that agreement on limiting the error packet handling.
In short, are you shooting js/smart-http-detect-remote-error topic
down and replacing it with this one?
As that topic is not yet in 'next', I am perfectly fine doing that.
I just want to make sure that is what you meant, as my reading of
[4] was a bit fuzzy.
Thanks.
>
> [1]: https://public-inbox.org/git/20181227065210.60817-1-masayasuzuki@google.com/
> [2]: https://public-inbox.org/git/20181127045301.103807-1-masayasuzuki@google.com/
> [3]: https://public-inbox.org/git/df7d3659ae5f11d163f1e992f3b9403be709ddb7.1544572142.git.steadmon@google.com/
> [4]: https://public-inbox.org/git/20181213221826.GE37614@google.com/
>
> Masaya Suzuki (2):
> Use packet_reader instead of packet_read_line
> pack-protocol.txt: accept error packets in any context
^ permalink raw reply
* Re: [Patch v5 3/4] git-compat-util.h: add FLOSS headers for HPE NonStop
From: Junio C Hamano @ 2019-01-03 22:09 UTC (permalink / raw)
To: Randall S. Becker
Cc: 'Eric Sunshine', randall.s.becker, 'Git List',
'Randall S. Becker'
In-Reply-To: <007a01d4a3b0$8ae10000$a0a30000$@nexbridge.com>
"Randall S. Becker" <rsbecker@nexbridge.com> writes:
> On January 3, 2019 16:41, Eric Sunshine wrote:
>> On Thu, Jan 3, 2019 at 4:04 PM <randall.s.becker@rogers.com> wrote:
>> > The HPE NonStop (a.k.a. __TANDEM) platform cannot build git without
>> > using the FLOSS package supplied by HPE. The convenient location for
>> > including the relevant headers is in this file.
>> >
>> > The NSIG define is also not defined on __TANDEM, so we define it here
>> > as 100 if it is not defined only for __TANDEM builds.
>> >
>> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> > ---
>> > diff --git a/git-compat-util.h b/git-compat-util.h @@ -397,6 +397,17
>> > @@ static inline char *git_find_last_dir_sep(const char *path)
>> > +#ifdef __TANDEM
>> > +#include <floss.h(floss_execl,floss_execlp,floss_execv,floss_execvp)>
>> > +#include <floss.h(floss_getpwuid)>
>> > +#ifndef NSIG
>> > +/* NonStop NSE and NSX do not provide NSIG. SIGGUARDIAN(99) is the
>> highest
>> > + known, by detective work using kill -l as a list is all signals
>> > + instead of signal.h where it should be. */
>>
>> Style nit: With two minor exceptions, all other multi-line comments in this file
>> are formatted as:
>>
>> /*
>> * Multi-line
>> * comment.
>> */
>
> Doh! I just missed that one. I'll fix this on the next round, if that's ok.
Again, I've queued with a tweak already, i.e. no need to reroll only
to fix this one.
Thanks, both.
^ permalink raw reply
* Re: [Patch v5 2/4] config.mak.uname: support for modern HPE NonStop config.
From: Junio C Hamano @ 2019-01-03 22:09 UTC (permalink / raw)
To: Randall S. Becker
Cc: 'Eric Sunshine', randall.s.becker, 'Git List',
'Randall S. Becker'
In-Reply-To: <007901d4a3b0$5dd1d030$19757090$@nexbridge.com>
"Randall S. Becker" <rsbecker@nexbridge.com> writes:
> On January 3, 2019 16:38, Eric Sunshine wrote:
>> On Thu, Jan 3, 2019 at 4:04 PM <randall.s.becker@rogers.com> wrote:
>> > A number of configuration options are not automatically detected by
>> > configure mechanisms, including the location of Perl and Python.
>> >
>> > There was a problem at a specific set of operating system versions
>> > that caused getopt to have compile errors. Account for this by
>> > providing emulation defines for those versions.
>> >
>> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> > ---
>> > diff --git a/config.mak.uname b/config.mak.uname @@ -470,7 +487,7
>> @@
>> > ifeq ($(uname_S),NONSTOP_KERNEL)
>> > NO_MKDTEMP = YesPlease
>> > OLD_ICONV = UnfortunatelyYes
>> > - NO_REGEX = YesPlease
>> > + NO_REGEX=NeedsStartEnd
>> > NO_PTHREADS = UnfortunatelyYes
>>
>> Style nit (probably not worth a re-roll): you lost the whitespace surrounding
>> '='
>
> I can fix this one in the next round of changes, or after this gets in, if that works, or signoff on this being fixed in place.
I've already tweaked this while queuing. i.e. no need to resend
only to fix this one.
Thanks, both.
^ permalink raw reply
* RE: [Patch v5 3/4] git-compat-util.h: add FLOSS headers for HPE NonStop
From: Randall S. Becker @ 2019-01-03 22:06 UTC (permalink / raw)
To: 'Eric Sunshine', randall.s.becker
Cc: 'Git List', 'Randall S. Becker'
In-Reply-To: <CAPig+cSdheCx-Ucnmg5eSXyNhyGPFF3L6N_Nc5uXQdc8Ytx2DA@mail.gmail.com>
On January 3, 2019 16:41, Eric Sunshine wrote:
> On Thu, Jan 3, 2019 at 4:04 PM <randall.s.becker@rogers.com> wrote:
> > The HPE NonStop (a.k.a. __TANDEM) platform cannot build git without
> > using the FLOSS package supplied by HPE. The convenient location for
> > including the relevant headers is in this file.
> >
> > The NSIG define is also not defined on __TANDEM, so we define it here
> > as 100 if it is not defined only for __TANDEM builds.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> > diff --git a/git-compat-util.h b/git-compat-util.h @@ -397,6 +397,17
> > @@ static inline char *git_find_last_dir_sep(const char *path)
> > +#ifdef __TANDEM
> > +#include <floss.h(floss_execl,floss_execlp,floss_execv,floss_execvp)>
> > +#include <floss.h(floss_getpwuid)>
> > +#ifndef NSIG
> > +/* NonStop NSE and NSX do not provide NSIG. SIGGUARDIAN(99) is the
> highest
> > + known, by detective work using kill -l as a list is all signals
> > + instead of signal.h where it should be. */
>
> Style nit: With two minor exceptions, all other multi-line comments in this file
> are formatted as:
>
> /*
> * Multi-line
> * comment.
> */
Doh! I just missed that one. I'll fix this on the next round, if that's ok.
^ permalink raw reply
* Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files
From: Anthony Sottile @ 2019-01-03 22:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Ben Peart, Git Mailing List
In-Reply-To: <xmqqwonlfm0g.fsf@gitster-ct.c.googlers.com>
On Thu, Jan 3, 2019 at 1:51 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Anthony Sottile <asottile@umich.edu> writes:
>
> > On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> A "fix" to Ben's optimization for this particular case should be
> >> fairly straight-forward. I think we have a special case in the
> >> checkout codepath for an initial checkout and disable "carry forward
> >> the fact that the user wanted all the paths removed", so it would be
> >> the matter of adding yet another condition (is_cache_unborn(), which
> >> is used to set topts.initial_checkout) to the large collection of
> >> conditions in skip_merge_working_tree().
> >
> > I think it might be simpler than that even -- the optimization treats
> > the following as equivalent when the current checked out revision is
> > deadbeef (even if the index / worktree differ), when before they were
> > not:
> >
> > - git checkout -b newbranch
> > - git checkout deadbeef -b newbranch
> >
> > If a revision is specified on the commandline it should be checked out.
>
> If it were to be a "fix", the exact same command line as people used
> to be able to use, i.e. "git checkout -b newbranch", should be made
> to do what it used to do.
>
> Forcing users to use a different command to workaround the bug is
> not a usable "fix". If we want a working workaround, you can tell
> your users to use
>
> git reset --hard HEAD && git checkout -b newbranch
>
> and that would already work without any code change ;-).
>
>
oh wow, I didn't realize `git checkout -b newbranch` also used to
reset the `--no-checkout` state, yeah you're right the optimization is
way more problematic than I had considered.
I'm working around by not using `--no-checkout` personally
Anthony
^ permalink raw reply
* RE: [Patch v5 2/4] config.mak.uname: support for modern HPE NonStop config.
From: Randall S. Becker @ 2019-01-03 22:04 UTC (permalink / raw)
To: 'Eric Sunshine', randall.s.becker
Cc: 'Git List', 'Randall S. Becker'
In-Reply-To: <CAPig+cSkok+QttWZTMngCdUrxHiO0rg53M4q=1q1vsewn7Qy_Q@mail.gmail.com>
On January 3, 2019 16:38, Eric Sunshine wrote:
> On Thu, Jan 3, 2019 at 4:04 PM <randall.s.becker@rogers.com> wrote:
> > A number of configuration options are not automatically detected by
> > configure mechanisms, including the location of Perl and Python.
> >
> > There was a problem at a specific set of operating system versions
> > that caused getopt to have compile errors. Account for this by
> > providing emulation defines for those versions.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> > diff --git a/config.mak.uname b/config.mak.uname @@ -470,7 +487,7
> @@
> > ifeq ($(uname_S),NONSTOP_KERNEL)
> > NO_MKDTEMP = YesPlease
> > OLD_ICONV = UnfortunatelyYes
> > - NO_REGEX = YesPlease
> > + NO_REGEX=NeedsStartEnd
> > NO_PTHREADS = UnfortunatelyYes
>
> Style nit (probably not worth a re-roll): you lost the whitespace surrounding
> '='
I can fix this one in the next round of changes, or after this gets in, if that works, or signoff on this being fixed in place.
^ permalink raw reply
* Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files
From: Junio C Hamano @ 2019-01-03 21:51 UTC (permalink / raw)
To: Anthony Sottile; +Cc: Duy Nguyen, Ben Peart, Git Mailing List
In-Reply-To: <CA+dzEB=+ROLVjp36SQjucouc8YUWTvYBrN4QyS5fsStMPtbw_w@mail.gmail.com>
Anthony Sottile <asottile@umich.edu> writes:
> On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>> A "fix" to Ben's optimization for this particular case should be
>> fairly straight-forward. I think we have a special case in the
>> checkout codepath for an initial checkout and disable "carry forward
>> the fact that the user wanted all the paths removed", so it would be
>> the matter of adding yet another condition (is_cache_unborn(), which
>> is used to set topts.initial_checkout) to the large collection of
>> conditions in skip_merge_working_tree().
>
> I think it might be simpler than that even -- the optimization treats
> the following as equivalent when the current checked out revision is
> deadbeef (even if the index / worktree differ), when before they were
> not:
>
> - git checkout -b newbranch
> - git checkout deadbeef -b newbranch
>
> If a revision is specified on the commandline it should be checked out.
If it were to be a "fix", the exact same command line as people used
to be able to use, i.e. "git checkout -b newbranch", should be made
to do what it used to do.
Forcing users to use a different command to workaround the bug is
not a usable "fix". If we want a working workaround, you can tell
your users to use
git reset --hard HEAD && git checkout -b newbranch
and that would already work without any code change ;-).
^ permalink raw reply
* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
From: Junio C Hamano @ 2019-01-03 21:45 UTC (permalink / raw)
To: Philip Oakley
Cc: Stephen P. Smith, git, Linus Torvalds,
Ævar Arnfjörð Bjarmason
In-Reply-To: <a5412274-028f-3662-e4f5-dbbcad4d9a40@iee.org>
Philip Oakley <philipoakley@iee.org> writes:
> On 02/01/2019 18:15, Junio C Hamano wrote:
>> We perhaps can use "test-tool date timestamp", like so
>>
>> check_human_date $(test-tool date timestamp "18000 seconds ago") ...
>>
>> or moving the part that munges 18000 into the above form inside
>> check_human_date helper function, e.g.
>>
>> check_human_date () {
>> commit_date=$(test-tool date timestamp "$1 seconds ago")
>> commit_date="$commit_date +0200"
>> expect=$2
>> ...
>> }
>>
>> which would let us write
>>
>> check_human_date 432000 "$THIS_YEAR_REGEX" # 5 days ago
>
>
> Just a quick bikeshed: if used, would this have a year end 5 day
> roll-over error potential, or will it always use the single date?
Hmph, interesting point. Indeed, date.c::show_date_normal() decides
to hide the year portion if the timestamp and the current time share
the same year, so on Thu Jan 3rd, an attempt to show a commit made
on Mon Dec 31st of the same week would end up showing the year, so
yes, I agree with you that the above would break.
+TODAY_REGEX='5 hours ago'
+THIS_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [012][0-9]:[0-6][0-9]'
+MORE_THAN_A_YEAR_REGEX='[A-Z][a-z][a-z] [A-Z][a-z][a-z] [0-9]* [0-9][0-9][0-9][0-9]'
>
> (I appreciate it is just suggestion code, not tested)
^ permalink raw reply
* Re: [Patch v5 3/4] git-compat-util.h: add FLOSS headers for HPE NonStop
From: Eric Sunshine @ 2019-01-03 21:41 UTC (permalink / raw)
To: randall.s.becker; +Cc: Git List, Randall S. Becker, Randall S . Becker
In-Reply-To: <20190103210351.13920-4-randall.s.becker@rogers.com>
On Thu, Jan 3, 2019 at 4:04 PM <randall.s.becker@rogers.com> wrote:
> The HPE NonStop (a.k.a. __TANDEM) platform cannot build git without
> using the FLOSS package supplied by HPE. The convenient location
> for including the relevant headers is in this file.
>
> The NSIG define is also not defined on __TANDEM, so we define it
> here as 100 if it is not defined only for __TANDEM builds.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> diff --git a/git-compat-util.h b/git-compat-util.h
> @@ -397,6 +397,17 @@ static inline char *git_find_last_dir_sep(const char *path)
> +#ifdef __TANDEM
> +#include <floss.h(floss_execl,floss_execlp,floss_execv,floss_execvp)>
> +#include <floss.h(floss_getpwuid)>
> +#ifndef NSIG
> +/* NonStop NSE and NSX do not provide NSIG. SIGGUARDIAN(99) is the highest
> + known, by detective work using kill -l as a list is all signals
> + instead of signal.h where it should be. */
Style nit: With two minor exceptions, all other multi-line comments in
this file are formatted as:
/*
* Multi-line
* comment.
*/
^ permalink raw reply
* Re: [Patch v5 2/4] config.mak.uname: support for modern HPE NonStop config.
From: Eric Sunshine @ 2019-01-03 21:38 UTC (permalink / raw)
To: randall.s.becker; +Cc: Git List, Randall S. Becker, Randall S . Becker
In-Reply-To: <20190103210351.13920-3-randall.s.becker@rogers.com>
On Thu, Jan 3, 2019 at 4:04 PM <randall.s.becker@rogers.com> wrote:
> A number of configuration options are not automatically detected by
> configure mechanisms, including the location of Perl and Python.
>
> There was a problem at a specific set of operating system versions
> that caused getopt to have compile errors. Account for this by
> providing emulation defines for those versions.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> diff --git a/config.mak.uname b/config.mak.uname
> @@ -470,7 +487,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> NO_MKDTEMP = YesPlease
> OLD_ICONV = UnfortunatelyYes
> - NO_REGEX = YesPlease
> + NO_REGEX=NeedsStartEnd
> NO_PTHREADS = UnfortunatelyYes
Style nit (probably not worth a re-roll): you lost the whitespace
surrounding '='
^ 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