* Re: git checkout crashes after server being updated to Debian X86_64
From: René Scharfe @ 2016-10-18 16:42 UTC (permalink / raw)
To: Raffael Reichelt, git
In-Reply-To: <6B2BE996-F696-4EB4-91CA-849D40B8802D@gmail.com>
Am 18.10.2016 um 17:17 schrieb Raffael Reichelt:
> Hello!
>
> I have a serious problem with git, After my provider had updated to a
> X86_64 architecture git crashes with various memory-related errors.
> This is happening remote when pushing to the repository from my local
> machine as well as trying it on a shell on the server itself.
>
> This are the error-messages:
>
> fatal: Out of memory, realloc failed
> fatal: recursion detected in die handler
> fatal: recursion detected in die handler
>
> or
> fatal: unable to create threaded lstat
> fatal: recursion detected in die handler
> or
> fatal: unable to create threaded lstat
> *** Error in `git': double free or corruption (fasttop): 0x0000000000a8ade0 ***
> fatal: recursion detected in die handler
> Aborted
>
> It’s obviously not a problem of the repository - happens with all of
> them. I think it is also not a question of size - happens with a 80M
> Repository as well as with a 500M one.
>
> Any way: did a
>
> git fsck
> Prüfe Objekt-Verzeichnisse: 100% (256/256), Fertig.
> Prüfe Objekte: 100% (56305/56305), Fertig.
>
> git gc --auto --prune=today —aggressive
> git repack
>
> Additionally I played around some config parameters so my config now looks like:
> [http]
> postbuffer = 524288000
> [pack]
> threads = 1
> deltaCacheSize = 128m
> packSizeLimit = 128m
> windowMemory = 128m
> [core]
> packedGitLimit = 128m
> packedGitWindowSize = 128m
> repositoryformatversion = 0
> filemode = true
> bare = true
>
> I am running
> git version 2.1.4
>
> on
> Linux infongp-de65 3.14.0-ui16196-uiabi1-infong-amd64 #1 SMP Debian 3.14.73-2~ui80+4 (2016-07-13) x86_64 GNU/Linux
>
> Anyone out there to help me getting out of this trouble?
Git 2.1.4 is the version that comes with Debian stable according to
https://packages.debian.org/jessie/git, so I guess using a more recent
version is not a reasonable option.
What do "file $(which git)" and "ulimit -a" return? Do you have an
x86-64 binary and no unnecessarily low limits set?
René
^ permalink raw reply
* Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block
From: Junio C Hamano @ 2016-10-18 16:36 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Stefan Beller, git@vger.kernel.org, Jakub Narębski
In-Reply-To: <cb622421-1857-d136-2618-ec62d438790d@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>> * rs/c-auto-resets-attributes:
>> pretty: avoid adding reset for %C(auto) if output is empty
>>
>> And neither of the two colon containing line remotely resembles how
>> a typical RFC-822 header is formatted. So that may serve as a hint
>> to how we can tighten it without introducing false negative.
>
> The only "offending" character is the space (according to RFC 822),
> but that sounds like a good rule to have.
I suspect that we should be willing to deviate from the letter of
RFC to reject misidentification. I saw things like
Thanks to: Jonathan Tan <jt@host.xz>
Signed-off-by: A U Thor <au@th.or>
in the wild (notice the SP between Thanks and to), for example.
Rejecting leading whitespace as a line that does *not* start the
header (hence its colon does not count) may be a good compromise.
> I think that "Signed-off-by:" is not guaranteed to be
> present.
But do we really care about that case where there is no S-o-b:? I
personally do not think so.
> Defining a trailer line as "a line starting with a token,
> then optional whitespace, then separator", maybe the following rule:
> - at least one trailer line generated by Git ("(cherry picked by" or
> "Signed-off-by") or configured in the "trailer" section in gitconfig
> OR
> - at least 3/4 logical trailer lines (I'm wondering if this should be
> 100% trailer lines)
I'd strongly suggest turning that OR to AND. We will not safely be
able to write a commit log message that describes how S-o-b lines
are handled in its last paragraph otherwise.
I do not care too deeply about 3/4, but I meant to allow 75% cruft
but no more than that, and the fact that the threashold is set at
way more than 50% is important. IOW, if you have
Ordinary log message here...
S-o-b: A U Thor <au@th.or>
[a short description that is typically a single liner
in the real world use pattern we saw in the world, but
could overflow to become multi line cruft]
S-o-b: R E Layer <re@lay.er>
"last paragraph" is 5 lines long, among which 60% are cruft that is
below the 75% threshold, and "am -s" can still add the S-o-b of the
committer at the end of that existing last paragraph. Making it too
strict would raise the false negative ratio.
^ permalink raw reply
* Re: [PATCH v3 4/6] trailer: make args have their own struct
From: Junio C Hamano @ 2016-10-18 16:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jonathan Tan, git@vger.kernel.org, Jakub Narębski
In-Reply-To: <CAGZ79kYuaHvO-aXeCocDseTLe13g=PesKxQgOrcOREtM1CsWcg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> @@ -29,6 +29,12 @@ struct trailer_item {
>> struct list_head list;
>> char *token;
>> char *value;
>> +};
>> +
>> +struct arg_item {
>> + struct list_head list;
>> + char *token;
>> + char *value;
>> struct conf_info conf;
>> };
>
> (Unrelated side note:) When first seeing this diff, I assumed the diff
> heuristic is going wild, because it doesn't add a full struct.
> But on a second closer look, I realize this is the only correct diff,
> because we do not account for moved lines from one struct to the
> other.
It probably is not "the only" correct diff, as you actually could
shift it the other way by one to three lines. I am not sure which
one among four possible diff is the easiest to grok, though. Both
the above (picking the highest possible position) and the below (the
other extreme) are probably easier to read than anything in between.
struct trailer_item {
+ struct list_head list;
+ char *token;
+ char *value;
+};
+
+struct arg_item {
struct list_head list;
char *token;
char *value;
struct conf_info conf;
};
^ permalink raw reply
* Re: [PATCH v3 16/25] sequencer: support amending commits
From: Junio C Hamano @ 2016-10-18 15:56 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610181352010.197091@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Mon, 17 Oct 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>> > This teaches the run_git_commit() function to take an argument that will
>> > allow us to implement "todo" commands that need to amend the commit
>> > messages ("fixup", "squash" and "reword").
>>
>> Likewise to 15/25, i.e. Good, though the growth by these two steps
>> starts to make me wonder if these three options should be crammed
>> into an unsigned "flags" bitword.
>
> After looking at the diff with the added complications of ORing and ANDing
> the flags, I'd much rather prefer to stay with the three flags being kept
> separately. It's not like we need to save bits, but we need to preserve
> readability as much as possible, I'd wager.
That's OK. I just wanted to make sure pros-and-cons have been
already considered.
The primary merit of using flags bitword is not to save bits; it is
done to limit the damage to the codebase when we need to add yet
another knob, by the way.
^ permalink raw reply
* Re: [PATCH v3 14/25] sequencer: introduce a helper to read files written by scripts
From: Junio C Hamano @ 2016-10-18 15:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610181339550.197091@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> In the meantime, I'd be happy to just add a comment that this function is
> intended for oneliners, but that it will also read multi-line files and
> only strip off the EOL marker from the last line.
>
> Would that work for you?
That would be ideal, I would think.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Junio C Hamano @ 2016-10-18 15:53 UTC (permalink / raw)
To: Santiago Torres; +Cc: git
In-Reply-To: <20161018133518.hwbv3gbeddjjfvcz@LykOS.localdomain>
Santiago Torres <santiago@nyu.edu> writes:
>> * st/verify-tag (2016-10-10) 7 commits
>> - t/t7004-tag: Add --format specifier tests
>> - t/t7030-verify-tag: Add --format specifier tests
>> - builtin/tag: add --format argument for tag -v
>> - builtin/verify-tag: add --format to verify-tag
>> - tag: add format specifier to gpg_verify_tag
>> - ref-filter: add function to print single ref_array_item
>> - gpg-interface, tag: add GPG_VERIFY_QUIET flag
>>
>> "git tag" and "git verify-tag" learned to put GPG verification
>> status in their "--format=<placeholders>" output format.
>>
>> Is this ready for 'next'?
>
> Hi, I saw this on the previous "what's cooking." Is there anything I
> need to do on my side to make sure this is ready for next?
Posting this exact message to the list would be an excellent way
;-).
Hopefully some competent reviewer comes and points me at a thread
where s/he says the series was already reviewed and in good shape
soonish, and your message may be a good trigger to make it happen.
^ permalink raw reply
* Re: [PATCH] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 15:24 UTC (permalink / raw)
To: Stefan Beller; +Cc: j6t, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <20161017221623.7299-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Currently a URL for the superproject ending in
>
> (A) .../path/to/dir
> (B) .../path/to/dir/
> (C) .../path/to/dir/.
> (D) .../path/to/dir/./.
> (E) .../path/to/dir/.///.//.
>
> is treated the same in (A) and (B), but (C, D, E) are different.
You may know what you meant to say with "treated", but the readers
do not know "treated" in what situation you are talking about. We
need to tell the readers that the bug being fixed is about resolving
a relative URL "../<something>" off of the URL of the superproject
to compute the remote URL for a submodule repository.
> We never produce the URLs in (C,D,E) ourselves, they come to use, because
> the user used it as the URL for cloning a superproject.
> Normalize these paths.
As you know the externally-visible impact of this change (which I
asked you, but didn't see an on-list answer to, by the way), please
describe what this means to the end user in the log message. It is
normally done in an earlier part of the log message to describe the
problem being solved and its background.
If I understand the issue correctly, it may go like this:
The remote URL for the submodule can be specified relative
to the URL of the superproject in .gitmodules. A top-level
git://site.xz/toplevel.git can specify in its .gitmodules
[submodule "sub"]
url = ../submodule.git
path = sub
to say that git://site.xz/submodule.git is where the
submodule bound at its "sub/" is found.
However, when the toplevel is cloned like this:
git clone git://site.xz/toplevel.git/. top
i.e. the toplevel specifies its URL with trailing "/.", the
code set the URL to git://site.xz/toplevel.git/submodule.git
for the submodule, which is nonsense. This was because the
logic failed to treat trailing "/." any differently from
trailing "/<anything-without-slash>" when resolving a
relative URL "../<something>" off of it. Stripping "/." at
the end does *not* take you one level up, even though
stripping "/<anything-without-slash>" does!
And then describe the solution/bugfix, listing A-C (or A-E) and
telling that these will be treated the same way.
^ permalink raw reply
* git checkout crashes after server being updated to Debian X86_64
From: Raffael Reichelt @ 2016-10-18 15:17 UTC (permalink / raw)
To: git
Hello!
I have a serious problem with git, After my provider had updated to a X86_64 architecture git crashes with various memory-related errors. This is happening remote when pushing to the repository from my local machine as well as trying it on a shell on the server itself.
This are the error-messages:
fatal: Out of memory, realloc failed
fatal: recursion detected in die handler
fatal: recursion detected in die handler
or
fatal: unable to create threaded lstat
fatal: recursion detected in die handler
or
fatal: unable to create threaded lstat
*** Error in `git': double free or corruption (fasttop): 0x0000000000a8ade0 ***
fatal: recursion detected in die handler
Aborted
It’s obviously not a problem of the repository - happens with all of them. I think it is also not a question of size - happens with a 80M Repository as well as with a 500M one.
Any way: did a
git fsck
Prüfe Objekt-Verzeichnisse: 100% (256/256), Fertig.
Prüfe Objekte: 100% (56305/56305), Fertig.
git gc --auto --prune=today —aggressive
git repack
Additionally I played around some config parameters so my config now looks like:
[http]
postbuffer = 524288000
[pack]
threads = 1
deltaCacheSize = 128m
packSizeLimit = 128m
windowMemory = 128m
[core]
packedGitLimit = 128m
packedGitWindowSize = 128m
repositoryformatversion = 0
filemode = true
bare = true
I am running
git version 2.1.4
on
Linux infongp-de65 3.14.0-ui16196-uiabi1-infong-amd64 #1 SMP Debian 3.14.73-2~ui80+4 (2016-07-13) x86_64 GNU/Linux
Anyone out there to help me getting out of this trouble?
Regards,
Raffael
^ permalink raw reply
* [PATCH] daemon, path.c: fix a bug with ~ in repo paths
From: Luke Shumaker @ 2016-10-18 15:06 UTC (permalink / raw)
To: git; +Cc: pclouds, peff, Luke Shumaker
The superficial aspect of this change is that git-daemon now allows paths
that start with a "~". Previously, if git-daemon was run with
"--base-path=/srv/git", it was impossible to get it to serve
"/srv/git/~foo/bar.git". An odd edge-case that was broken.
But from a source-code standpoint, the change is in path.c:enter_repo(). I
have adjusted it to take separate "strict_prefix" and "strict_suffix"
arguments, rather than a single "strict" argument.
I also make it clearer what the purpose of each path buffer is for, by
renaming them to chdir_path and ret_path; chdir_path is the path that we
pass to chdir(); return_path is the path we return to the user. Using this
nomenclature, we can more easily explain the behavior of the function.
There are 3 DWIM measures that enter_repo() provides: tilde expansion,
suffix guessing, and gitfile expansion; it also trims trailing slashes.
Here is how they are applied to each path:
+------------------------------+----------------+----------------+
| Before this commit | chdir_path | ret_path |
+------------------------------+----------------+----------------+
| trim trailing slashes | !strict | !strict |
| tilde expansion | !strict | false |
| suffix guessing | !strict | !strict |
| gitfile expansion (< 2.6.3) | !strict | false |
| gitfile expansion (>= 2.6.3) | true | strict |
+------------------------------+----------------+----------------+
| With this commit | chdir_path | ret_path |
+------------------------------+----------------+----------------+
| trim trailing slashes | true | true |
| tilde expansion | !strict_prefix | false |
| suffix guessing | !strict_suffix | !strict_suffix |
| gitfile expansion | true | false |
+------------------------------+----------------+----------------+
The separation of "strict" into "strict_prefix" and "strict_suffix" is
necessary for git-daemon because it has separate --strict-paths (affects
prefix and suffix) and --user-path (just prefix) flags that can be toggled
separately.
In the other programs where enter_repo() is called, I continued the
existing behavior of tying the prefix and suffix strictness together
together; though I am not entirely sure that they should all be enabling
tilde expansion. But for now, their behavior hasn't changed.
Signed-off-by: Luke Shumaker <lukeshu@sbcglobal.net>
---
builtin/receive-pack.c | 2 +-
builtin/upload-archive.c | 2 +-
cache.h | 2 +-
daemon.c | 42 +++++++--------
http-backend.c | 2 +-
path.c | 135 +++++++++++++++++++++++++----------------------
upload-pack.c | 2 +-
7 files changed, 96 insertions(+), 91 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 011db00..f430e96 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1860,7 +1860,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
setup_path();
- if (!enter_repo(service_dir, 0))
+ if (!enter_repo(service_dir, 0, 0))
die("'%s' does not appear to be a git repository", service_dir);
git_config(receive_pack_config, NULL);
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 2caedf1..00d4ced 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -25,7 +25,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
if (argc != 2)
usage(upload_archive_usage);
- if (!enter_repo(argv[1], 0))
+ if (!enter_repo(argv[1], 0, 0))
die("'%s' does not appear to be a git repository", argv[1]);
/* put received options in sent_argv[] */
diff --git a/cache.h b/cache.h
index 4cba08e..6380be0 100644
--- a/cache.h
+++ b/cache.h
@@ -1024,7 +1024,7 @@ enum scld_error safe_create_leading_directories_const(const char *path);
int mkdir_in_gitdir(const char *path);
extern char *expand_user_path(const char *path);
-const char *enter_repo(const char *path, int strict);
+const char *enter_repo(const char *path, int strict_prefix, int strict_suffix);
static inline int is_absolute_path(const char *path)
{
return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
diff --git a/daemon.c b/daemon.c
index 425aad0..118d337 100644
--- a/daemon.c
+++ b/daemon.c
@@ -170,27 +170,23 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
return NULL;
}
- if (*dir == '~') {
- if (!user_path) {
- logerror("'%s': User-path not allowed", dir);
- return NULL;
- }
- if (*user_path) {
- /* Got either "~alice" or "~alice/foo";
- * rewrite them to "~alice/%s" or
- * "~alice/%s/foo".
- */
- int namlen, restlen = strlen(dir);
- const char *slash = strchr(dir, '/');
- if (!slash)
- slash = dir + restlen;
- namlen = slash - dir;
- restlen -= namlen;
- loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
- snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
- namlen, dir, user_path, restlen, slash);
- dir = rpath;
- }
+ if (*dir == '~' && *user_path && user_path[0] != '\0') {
+ /* Got either "~alice" or "~alice/foo";
+ * rewrite them:
+ *
+ * ~alice -> ~alice/user_dir
+ * ~alice/foo -> ~alice/user_dir/foo
+ */
+ int namlen, restlen = strlen(dir);
+ const char *slash = strchr(dir, '/');
+ if (!slash)
+ slash = dir + restlen;
+ namlen = slash - dir;
+ restlen -= namlen;
+ loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
+ snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
+ namlen, dir, user_path, restlen, slash);
+ dir = rpath;
}
else if (interpolated_path && hi->saw_extended_args) {
struct strbuf expanded_path = STRBUF_INIT;
@@ -223,14 +219,14 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
dir = rpath;
}
- path = enter_repo(dir, strict_paths);
+ path = enter_repo(dir, strict_paths || !user_path, strict_paths);
if (!path && base_path && base_path_relaxed) {
/*
* if we fail and base_path_relaxed is enabled, try without
* prefixing the base path
*/
dir = directory;
- path = enter_repo(dir, strict_paths);
+ path = enter_repo(dir, strict_paths || !user_path, strict_paths);
}
if (!path) {
diff --git a/http-backend.c b/http-backend.c
index adc8c8c..d71ed81 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -697,7 +697,7 @@ int cmd_main(int argc, const char **argv)
not_found(&hdr, "Request not supported: '%s'", dir);
setup_path();
- if (!enter_repo(dir, 0))
+ if (!enter_repo(dir, 0, 0))
not_found(&hdr, "Not a git repository: '%s'", dir);
if (!getenv("GIT_HTTP_EXPORT_ALL") &&
access("git-daemon-export-ok", F_OK) )
diff --git a/path.c b/path.c
index fe3c4d9..636349e 100644
--- a/path.c
+++ b/path.c
@@ -646,96 +646,105 @@ char *expand_user_path(const char *path)
}
/*
- * First, one directory to try is determined by the following algorithm.
+ * chdir() to, and set_git_dir() to the directory found with "path", using the
+ * following algorithm.
*
- * (0) If "strict" is given, the path is used as given and no DWIM is
- * done. Otherwise:
- * (1) "~/path" to mean path under the running user's home directory;
- * (2) "~user/path" to mean path under named user's home directory;
- * (3) "relative/path" to mean cwd relative directory; or
- * (4) "/absolute/path" to mean absolute directory.
+ * (0) "relative/path" to mean cwd relative directory; or
+ * (1) "/absolute/path" to mean absolute directory.
+ * (2) trim trailing slashes
*
- * Unless "strict" is given, we check "%s/.git", "%s", "%s.git/.git", "%s.git"
- * in this order. We select the first one that is a valid git repository, and
- * chdir() to it. If none match, or we fail to chdir, we return NULL.
+ * Unless "strict_prefix" is given:
+ * (3) "~/path" to mean path under the running user's home directory;
+ * (4) "~user/path" to mean path under named user's home directory;
*
- * If all goes well, we return the directory we used to chdir() (but
- * before ~user is expanded), avoiding getcwd() resolving symbolic
- * links. User relative paths are also returned as they are given,
- * except DWIM suffixing.
+ * Unless "strict_suffix" is given:
+ * (5) check "%s/.git", "%s", "%s.git/.git", "%s.git" in this order. We select
+ * the first one that is a valid git repository, and chdir() to it. If none
+ * match, we return NULL.
+ *
+ * And then, unconditionally:
+ * (6) If the result is a .git file (instead of a directory) that points to a
+ * directory elsewhere, follow it.
+ *
+ * If all goes well, we return the input path with suffix alteration (steps 2,
+ * 5, 6) applied, but without prefix alteration (user paths) applied. The
+ * returned value is a pointer to a static buffer.
*/
-const char *enter_repo(const char *path, int strict)
+const char *enter_repo(const char *path, int strict_prefix, int strict_suffix)
{
- static struct strbuf validated_path = STRBUF_INIT;
- static struct strbuf used_path = STRBUF_INIT;
+ /* chdir_path is the path we chdir() to */
+ static struct strbuf chdir_path = STRBUF_INIT;
+ /* ret_path is the path we return */
+ static struct strbuf ret_path = STRBUF_INIT;
+
+ int len;
+ const char *gitfile;
if (!path)
return NULL;
- if (!strict) {
+ len = strlen(path);
+
+ /* strip trailing slashes */
+ while ((1 < len) && (path[len-1] == '/'))
+ len--;
+
+ /*
+ * We can handle arbitrary-sized buffers, but this remains as a
+ * sanity check on untrusted input.
+ */
+ if (PATH_MAX <= len)
+ return NULL;
+
+ strbuf_reset(&chdir_path);
+ strbuf_add(&chdir_path, path, len);
+ strbuf_reset(&ret_path);
+ strbuf_add(&ret_path, path, len);
+
+ if (!strict_prefix && chdir_path.buf[0] == '~') {
+ /* operate only on chdir_path */
+ char *newpath = expand_user_path(chdir_path.buf);
+ if (!newpath)
+ return NULL;
+ strbuf_attach(&chdir_path, newpath, strlen(newpath),
+ strlen(newpath));
+ }
+
+ if (!strict_suffix) {
+ /* operate on both chdir_path and ret_path */
static const char *suffix[] = {
"/.git", "", ".git/.git", ".git", NULL,
};
- const char *gitfile;
- int len = strlen(path);
int i;
- while ((1 < len) && (path[len-1] == '/'))
- len--;
-
- /*
- * We can handle arbitrary-sized buffers, but this remains as a
- * sanity check on untrusted input.
- */
- if (PATH_MAX <= len)
- return NULL;
-
- strbuf_reset(&used_path);
- strbuf_reset(&validated_path);
- strbuf_add(&used_path, path, len);
- strbuf_add(&validated_path, path, len);
-
- if (used_path.buf[0] == '~') {
- char *newpath = expand_user_path(used_path.buf);
- if (!newpath)
- return NULL;
- strbuf_attach(&used_path, newpath, strlen(newpath),
- strlen(newpath));
- }
for (i = 0; suffix[i]; i++) {
struct stat st;
- size_t baselen = used_path.len;
- strbuf_addstr(&used_path, suffix[i]);
- if (!stat(used_path.buf, &st) &&
+ size_t baselen = chdir_path.len;
+ strbuf_addstr(&chdir_path, suffix[i]);
+ if (!stat(chdir_path.buf, &st) &&
(S_ISREG(st.st_mode) ||
- (S_ISDIR(st.st_mode) && is_git_directory(used_path.buf)))) {
- strbuf_addstr(&validated_path, suffix[i]);
+ (S_ISDIR(st.st_mode) && is_git_directory(chdir_path.buf)))) {
+ strbuf_addstr(&ret_path, suffix[i]);
break;
}
- strbuf_setlen(&used_path, baselen);
+ strbuf_setlen(&chdir_path, baselen);
}
if (!suffix[i])
return NULL;
- gitfile = read_gitfile(used_path.buf);
- if (gitfile) {
- strbuf_reset(&used_path);
- strbuf_addstr(&used_path, gitfile);
- }
- if (chdir(used_path.buf))
- return NULL;
- path = validated_path.buf;
}
- else {
- const char *gitfile = read_gitfile(path);
- if (gitfile)
- path = gitfile;
- if (chdir(path))
- return NULL;
+
+ gitfile = read_gitfile(chdir_path.buf);
+ if (gitfile) {
+ /* operate only on chdir_path */
+ strbuf_reset(&chdir_path);
+ strbuf_addstr(&chdir_path, gitfile);
}
+ if (chdir(chdir_path.buf))
+ return NULL;
if (is_git_directory(".")) {
set_git_dir(".");
check_repository_format();
- return path;
+ return ret_path.buf;
}
return NULL;
diff --git a/upload-pack.c b/upload-pack.c
index ca7f941..c73b776 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -859,7 +859,7 @@ int cmd_main(int argc, const char **argv)
dir = argv[0];
- if (!enter_repo(dir, strict))
+ if (!enter_repo(dir, strict, strict))
die("'%s' does not appear to be a git repository", dir);
git_config(upload_pack_config, NULL);
--
2.10.0
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Santiago Torres @ 2016-10-18 13:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq1szm8ukf.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 735 bytes --]
> * st/verify-tag (2016-10-10) 7 commits
> - t/t7004-tag: Add --format specifier tests
> - t/t7030-verify-tag: Add --format specifier tests
> - builtin/tag: add --format argument for tag -v
> - builtin/verify-tag: add --format to verify-tag
> - tag: add format specifier to gpg_verify_tag
> - ref-filter: add function to print single ref_array_item
> - gpg-interface, tag: add GPG_VERIFY_QUIET flag
>
> "git tag" and "git verify-tag" learned to put GPG verification
> status in their "--format=<placeholders>" output format.
>
> Is this ready for 'next'?
Hi, I saw this on the previous "what's cooking." Is there anything I
need to do on my side to make sure this is ready for next?
Thanks!
-Santiago.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Git log exclude/remotes/branches options not working as expected
From: Robert Dailey @ 2016-10-18 13:33 UTC (permalink / raw)
To: Git
I have 3 remotes registered in my clone:
origin, fork, drive
When I do:
$ git log --oneline --decorate --graph
I only want to see branches under:
refs/heads/*
refs/remotes/origin/*
I tried the following:
$ git log --oneline --decorate --graph --simplify-by-decoration
--remote=origin topic1..master
However, I still see refs present in the graph for 'drive' and 'fork'
remote tracking branches. I can't tell if these are shown simply
because other refs not excluded by my options happen to also be at
that SHA1, or if the log command is still generating the graph based
on other branches.
What I'm expecting is that I literally see NONE of those excluded refs
on the graph, even if other included refs also happen to be positioned
at those commits. It's the visualization of the refs I'm concerned
about: I have a lot of remote tracking branches in those remotes that
clutter the view; I'd rather not see them at all (in addition to the
graph not considering them when it is being built/generated). Am I
misunderstanding the purpose here? How can I achieve my goals?
Documentation hasn't really helped me out here.
^ permalink raw reply
* Re: Merge conflicts in .gitattributes can cause trouble
From: Johannes Schindelin @ 2016-10-18 12:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, git, Jeff King, me
In-Reply-To: <xmqqvawqvp6y.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 17 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > I would vote for:
> >
> > 4. We keep letting Git read in the *current* version of .gitattributes
> > *before* the merge, and apply those attributes while performing the
> > merge.
>
> Even though this needs a major surgery to the way the attr subsystem
> reads from these files, I think it is conceptually the cleanest.
To the contrary. As far as I can see, when calling `git merge`, Git
currently *does* read .gitattributes from the file, and if that fails,
falls back to reading that file from the index.
In other words, option 4. is the current behavior no change required.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v4 08/25] sequencer: completely revamp the "todo" script parsing
From: Johannes Schindelin @ 2016-10-18 12:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqqfunusj1d.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 17 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > - for (i = 1; *p; i++) {
> > + for (i = 1; *p; i++, p = next_p) {
> > char *eol = strchrnul(p, '\n');
> > - commit = parse_insn_line(p, eol, opts);
> > - if (!commit)
> > - return error(_("Could not parse line %d."), i);
> > - next = commit_list_append(commit, next);
> > - p = *eol ? eol + 1 : eol;
> > +
> > + next_p = *eol ? eol + 1 /* strip LF */ : eol;
>
> This one was explained as "skip LF" in the previous round, and that
> is more correct than "strip", I think. The +1 here is not done to
> "strip" the LF out of the end result, but to "skip" one to move to
> the beginning of the next line.
Changed,
Dscho
^ permalink raw reply
* Re: [PATCH v4 05/25] sequencer: eventually release memory allocated for the option values
From: Johannes Schindelin @ 2016-10-18 12:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqq4m4avlr7.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 17 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
> > like a one-shot command when it reads its configuration: memory is
> > allocated and released only when the command exits.
> >
> > This is kind of okay for git-cherry-pick, which *is* a one-shot
> > command. All the work to make the sequencer its work horse was
> > done to allow using the functionality as a library function, though,
> > including proper clean-up after use.
> >
> > To remedy that, we now take custody of the option values in question,
> > requiring those values to be malloc()ed or strdup()ed
>
> That is the approach this patch takes, so "eventually release" in
> the title is no longer accurate, I would think.
To the contrary, we now free() things in remove_state(), so we still
"eventually release" the memory.
> > Sadly, the current approach makes the code uglier, as we now have to
> > take care to strdup() the values passed via the command-line.
>
> I obviously disagree with that statement and the _entrust was too
> ugly to live, but it is obviously subjective, and it boils down to
> who has a better taste. Let's not go there.
Changed.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH v3 16/25] sequencer: support amending commits
From: Johannes Schindelin @ 2016-10-18 11:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqinsqx54y.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 17 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > This teaches the run_git_commit() function to take an argument that will
> > allow us to implement "todo" commands that need to amend the commit
> > messages ("fixup", "squash" and "reword").
>
> Likewise to 15/25, i.e. Good, though the growth by these two steps
> starts to make me wonder if these three options should be crammed
> into an unsigned "flags" bitword.
After looking at the diff with the added complications of ORing and ANDing
the flags, I'd much rather prefer to stay with the three flags being kept
separately. It's not like we need to save bits, but we need to preserve
readability as much as possible, I'd wager.
> I see you have v4, so I'll ignore the remainder of this stale round
> and start reading that updated one instead.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH v3 14/25] sequencer: introduce a helper to read files written by scripts
From: Johannes Schindelin @ 2016-10-18 11:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <xmqqr37ex5ck.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 17 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > +/*
> > + * Reads a file that was presumably written by a shell script, i.e.
> > + * with an end-of-line marker that needs to be stripped.
> > + *
> > + * Returns 1 if the file was read, 0 if it could not be read or does not exist.
> > + */
> > +static int read_oneliner(struct strbuf *buf,
> > + const char *path, int skip_if_empty)
> > +...
> > + if (strbuf_read_file(buf, path, 0) < 0) {
> > + warning_errno(_("could not read '%s'"), path);
> > + return 0;
> > + }
> > + if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
> > + if (--buf->len > orig_len && buf->buf[buf->len - 1] == '\r')
> > + --buf->len;
> > + buf->buf[buf->len] = '\0';
> > + }
>
> The name says "oneliner" but this reads the whole thing and trims
> only the last line of the input. Which is correct?
The latter. Basically, `read_oneliner()` is short-hand for "that thing
that shell does when you use `cat file` with backticks.
I do not like `read_stripping_last_eol()`, `read_what_the_shell_wrote()`
nor `read_skipping_last_lf()`. So if you come up with any brilliant idea,
I am all ears.
In the meantime, I'd be happy to just add a comment that this function is
intended for oneliners, but that it will also read multi-line files and
only strip off the EOL marker from the last line.
Would that work for you?
Dscho
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Johannes Schindelin @ 2016-10-18 11:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqq7f96ykkc.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 17 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> I'll mark it as "wait for follow-up fix" in whats-cooking.txt (on
> >> 'todo' branch) to remind myself not to merge it yet.
> >
> > May I request your guidance as to your preference how to proceed? ...
>
> I guess I didn't see this before I sent my response to the review
> thread, which was in my pile of "these need more thought than others
> before responding" topics.
>
> > Here are the options I see:
> >
> > A) remove the tests in question
> >
> > B) mark them as !MINGW instead
> >
> > C) change just those two tests from using `$PWD` (pseudo-Unix path) to
> > `$(pwd)` (native path)
> >
> > I would like to hear your feedback about your preference, but not without
> > priming you a little bit by detailing my current opinion on the matter:
> >
> > While I think B) would be the easiest to read, C) would document the
> > expected behavior better. A) would feel to me like shrugging, i.e. the
> > lazy, wrong thing to do.
> >
> > What do you think?
>
> As to my preference on tests, I guess what I suggested was a cross
> between your B and C below, and I can go with either one as an
> abbreviated version of my preference ;-)
>
> I am still wondering if the test is expecting the right behaviour,
> though. If some codepaths rely on a question "please resolve '../.'
> relative to 'path/to/dir/.'" being answered as "that's path/to/dir
> itself", it smells to me that the downstream of the dataflow that
> expects such an answer, as well as the machinery that produces such
> an answer, are acting as two wrongs that happen to cancel each
> other. Am I grossly misunderstanding what that test is doing?
I think your "let's take a step back" was spot on: when being passed a
path ending in "/." and being told to normalize the relative path "../."
on top, the very special meaning of "." should be taken into
consideration.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH] submodule--helper: normalize funny urls
From: Johannes Schindelin @ 2016-10-18 11:23 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Johannes Sixt, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kaE=KAWcDTEvAqGO=-zzjh3VeSwHH82SnhK6DO9THOyPA@mail.gmail.com>
Hi Stefan,
On Mon, 17 Oct 2016, Stefan Beller wrote:
> On Mon, Oct 17, 2016 at 3:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Stefan Beller <sbeller@google.com> writes:
> >
> >> +static void strip_url_ending(char *url, size_t *_len)
> >> +{
> >> + int check_url_stripping = 1;
> >> + size_t len = _len ? *_len : strlen(url);
> >> +
> >> + while (check_url_stripping) {
> >> + check_url_stripping = 0;
> >> + if (is_dir_sep(url[len-2]) && url[len-1] == '.') {
> >
> > This is "strip /. at the end" it seems.
> >
> > Does anything in the loop control guarantees 2 <= len at this point?
>
> Oh, thanks for pointing that out. I thought about that and missed to add it.
> I'll reroll with the length check once we hear back from Windows folks,
> that this is a viable strategy for them, too.
It is a viable strategy for me, too.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 0/4] diff.wsErrorHighlight configuration variable
From: Jeff King @ 2016-10-18 11:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, strk
In-Reply-To: <20161004225449.6759-1-gitster@pobox.com>
On Tue, Oct 04, 2016 at 03:54:45PM -0700, Junio C Hamano wrote:
> "git diff" and its family of commands have "--ws-error-highlight"
> option to allow whitespace breakages on old and context lines
> painted in color.diff.whitespace color, instead of the usual "we
> paint breakages only on new lines", but there wasn't a configuration
> variable that corresponds to it.
>
> This would be a lot closer to a series that could be acceptable,
> compared to the previous "it should look like this" patch.
>
> Junio C Hamano (4):
> t4015: split out the "setup" part of ws-error-highlight test
> diff.c: refactor parse_ws_error_highlight()
> diff.c: move ws-error-highlight parsing helpers up
> diff: introduce diff.wsErrorHighlight option
This topic got stuck in my dreaded "to review" pile and I forgot about
it. I see you've already marked it for merging to master, but FWIW, I
read it over and did not see any problems.
-Peff
^ permalink raw reply
* Re: [PATCH] fetch: use "quick" has_sha1_file for tag following
From: Jeff King @ 2016-10-18 10:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Vegard Nossum, git, Quentin Casasnovas, Shawn Pearce,
Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqeg3ex4qz.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 17, 2016 at 10:30:28AM -0700, Junio C Hamano wrote:
> > It looks like I _did_ look into optimizing this into a single stat()
> > call in the thread at [1]. I completely forgot about that. I did find
> > there that naively using stat_validity() on a directory is racy, though
> > I wonder if we could do something clever with gettimeofday() instead.
>
> It feels funny to hear an idea to compare fs timestamp with gettimeofday
> immedately after hearing the word NFS, though ;-).
Yeah, I had a funny feeling in my stomach as I wrote that.
What you really want to know is the current filesystem time. You'd
probably have to do something gross like creating a new file and then
comparing its timestamp. In theory you'd only have to do that _once_,
and then as long as the pack directory wasn't changing, you could say "I
don't know what time it is now, but I know it is at least time X, and I
know that X is greater than Y, the pack directory timestamp, therefore
the pack directory hasn't changed since I last looked".
That assumes monotonic clocks, but we basically already do so for the
racy-git checks, I think.
I dunno. It feels...complicated. And bad to require writing to the
repository for what would otherwise be a read-only operation. But I
don't see any fundamental reason it could not work.
-Peff
^ permalink raw reply
* Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block
From: Jonathan Tan @ 2016-10-18 2:02 UTC (permalink / raw)
To: Junio C Hamano, Stefan Beller; +Cc: git@vger.kernel.org, Jakub Narębski
In-Reply-To: <xmqq7f96sa9i.fsf@gitster.mtv.corp.google.com>
On 10/17/2016 06:42 PM, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>>>
>>> Existing trailers are extracted from the input message by looking for
>>> -a group of one or more lines that contain a colon (by default), where
>>> +a group of one or more lines in which at least one line contains a
>>> +colon (by default), where
>>
>> Please see commit
>> 578e6021c0819d7be1179e05e7ce0e6fdb2a01b7
>> for an example where I think this is overly broad.
>
> Hmph. That's a merge.
>
> Merge branch 'rs/c-auto-resets-attributes'
>
> When "%C(auto)" appears at the very beginning of the pretty format
> string, it did not need to issue the reset sequence, but it did.
>
> * rs/c-auto-resets-attributes:
> pretty: avoid adding reset for %C(auto) if output is empty
>
> And neither of the two colon containing line remotely resembles how
> a typical RFC-822 header is formatted. So that may serve as a hint
> to how we can tighten it without introducing false negative.
The only "offending" character is the space (according to RFC 822), but
that sounds like a good rule to have.
>> Another made up example, that I'd want to feed
>> in commit -s eventually:
>>
>> --8<--
>> demonstrate colons in Java
>>
>> First paragraph is not interesting.
>>
>> Also if using another Language such as Java, where I point out
>> Class::function() to be problematic
>> --8<--
>>
>> This would lack the white space between the last paragraph and
>> the Sign off ?
>>
>> So for this patch I am mostly concerned about false positives hidden
>> in actual text.
>
> Yes.
>
> These are exactly why I mentioned "if certian number or percentage"
> in my earlier suggestion.
>
> I think in practice, "A paragraph with at least one Signed-off-by:
> line, and has no more than 3/4 of the (logical) lines that do not
> resemble how a typical RFC-822 header is formatted" or something
> along that line would give us a reasonable safety.
I think that "Signed-off-by:" is not guaranteed to be present. Defining
a trailer line as "a line starting with a token, then optional
whitespace, then separator", maybe the following rule:
- at least one trailer line generated by Git ("(cherry picked by" or
"Signed-off-by") or configured in the "trailer" section in gitconfig
OR
- at least 3/4 logical trailer lines (I'm wondering if this should be
100% trailer lines)
?
> Your Java example will fail the criteria in two ways, so we'd be
> safe ;-)
^ permalink raw reply
* Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block
From: Junio C Hamano @ 2016-10-18 1:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jonathan Tan, git@vger.kernel.org, Jakub Narębski
In-Reply-To: <CAGZ79kYLq1qA4_Qg2x5Fiu5AmGBZdozm4zk6K7LkU+uJ1LNUTw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>>
>> Existing trailers are extracted from the input message by looking for
>> -a group of one or more lines that contain a colon (by default), where
>> +a group of one or more lines in which at least one line contains a
>> +colon (by default), where
>
> Please see commit
> 578e6021c0819d7be1179e05e7ce0e6fdb2a01b7
> for an example where I think this is overly broad.
Hmph. That's a merge.
Merge branch 'rs/c-auto-resets-attributes'
When "%C(auto)" appears at the very beginning of the pretty format
string, it did not need to issue the reset sequence, but it did.
* rs/c-auto-resets-attributes:
pretty: avoid adding reset for %C(auto) if output is empty
And neither of the two colon containing line remotely resembles how
a typical RFC-822 header is formatted. So that may serve as a hint
to how we can tighten it without introducing false negative.
> Another made up example, that I'd want to feed
> in commit -s eventually:
>
> --8<--
> demonstrate colons in Java
>
> First paragraph is not interesting.
>
> Also if using another Language such as Java, where I point out
> Class::function() to be problematic
> --8<--
>
> This would lack the white space between the last paragraph and
> the Sign off ?
>
> So for this patch I am mostly concerned about false positives hidden
> in actual text.
Yes.
These are exactly why I mentioned "if certian number or percentage"
in my earlier suggestion.
I think in practice, "A paragraph with at least one Signed-off-by:
line, and has no more than 3/4 of the (logical) lines that do not
resemble how a typical RFC-822 header is formatted" or something
along that line would give us a reasonable safety.
Your Java example will fail the criteria in two ways, so we'd be
safe ;-)
^ permalink raw reply
* Re: [PATCH v3 6/6] trailer: support values folded to multiple lines
From: Stefan Beller @ 2016-10-18 0:55 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git@vger.kernel.org, Junio C Hamano, Jakub Narębski
In-Reply-To: <0a187897454a5bce946d675f6f6c93283b411f31.1476466609.git.jonathantanmy@google.com>
On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> Currently, interpret-trailers requires that a trailer be only on 1 line.
> For example:
>
> a: first line
> second line
>
> would be interpreted as one trailer line followed by one non-trailer line.
>
> Make interpret-trailers support RFC 822-style folding, treating those
> lines as one logical trailer.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
Looks good,
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block
From: Stefan Beller @ 2016-10-18 0:49 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git@vger.kernel.org, Junio C Hamano, Jakub Narębski
In-Reply-To: <1b3fe84e4b6126884a801e721d0a93c41fcb4184.1476466609.git.jonathantanmy@google.com>
On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>
> Existing trailers are extracted from the input message by looking for
> -a group of one or more lines that contain a colon (by default), where
> +a group of one or more lines in which at least one line contains a
> +colon (by default), where
Please see commit
578e6021c0819d7be1179e05e7ce0e6fdb2a01b7
for an example where I think this is overly broad.
Another made up example, that I'd want to feed
in commit -s eventually:
--8<--
demonstrate colons in Java
First paragraph is not interesting.
Also if using another Language such as Java, where I point out
Class::function() to be problematic
--8<--
This would lack the white space between the last paragraph and
the Sign off ?
So for this patch I am mostly concerned about false positives hidden
in actual text.
^ permalink raw reply
* Re: [PATCH v3 4/6] trailer: make args have their own struct
From: Stefan Beller @ 2016-10-17 23:20 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git@vger.kernel.org, Junio C Hamano, Jakub Narębski
In-Reply-To: <2357e9e6418fc5b30a60efe16dd8f72bf7544fef.1476466609.git.jonathantanmy@google.com>
On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> Improve type safety by making arguments (whether from configuration or
> from the command line) have their own "struct arg_item" type, separate
> from the "struct trailer_item" type used to represent the trailers in
> the buffer being manipulated.
>
> This change also prepares "struct trailer_item" to be further
> differentiated from "struct arg_item" in future patches.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> trailer.c | 135 +++++++++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 85 insertions(+), 50 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index 54cc930..a9ed3f8 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -29,6 +29,12 @@ struct trailer_item {
> struct list_head list;
> char *token;
> char *value;
> +};
> +
> +struct arg_item {
> + struct list_head list;
> + char *token;
> + char *value;
> struct conf_info conf;
> };
(Unrelated side note:) When first seeing this diff, I assumed the diff
heuristic is going wild, because it doesn't add a full struct.
But on a second closer look, I realize this is the only correct diff,
because we do not account for moved lines from one struct to the
other.
> static void add_arg_to_input_list(struct trailer_item *on_tok,
> - struct trailer_item *arg_tok)
> + struct arg_item *arg_tok)
> {
> - if (after_or_end(arg_tok->conf.where))
> - list_add(&arg_tok->list, &on_tok->list);
> + int aoe = after_or_end(arg_tok->conf.where);
> + struct trailer_item *to_add = trailer_from_arg(arg_tok);
> + if (aoe)
The use of an extra variable here is more readable than my
imagined version of inlining to_add into the list_add calls
just to save aoe.
Looks good to me.
Stefan
^ 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