Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives
On Tuesday 12 September 2017 08:59 PM, Jeff King wrote: Like all good writing rules, I think it's important to know when to break them. :) That's right. "Have guidelines but 'Be bold' enough to break them when they seem to be inducing counter productivity." Writing in the imperative is _most_ important in the subject. You're likely to see a lot of subjects in a list, and it makes the list easier to read if they all match. It also tends to be shorter, which is good for subjects. For short commit messages, I think the imperative also keeps things tight and to the point: describe the problem and then say how to fix it. The recent 0db3dc75f is a good example (which I picked by skimming recent "git log" output). But saying "this patch" is IMHO not that big a problem there, as long as it isn't done excessively. When you the explanation is longer or more complicated, the imperative can actually be a bit _too_ terse. In longer text it helps to guide readers in the direction you want their thoughts to take. Having a three-paragraph explanation of the problem or current state of things and then jumping right into "Do this. Do that." lacks context. A marker like "this patch" helps the reader know that you're switching gears to talking about the solution. I also think that "I" is useful in avoiding the passive voice. It can certainly be used gratuitously and make things less clear, but in most cases I'd rather see something like "I tested performance under these conditions" than "Performance was tested under these conditions". I also often use the "academic we" here even when I worked on something myself. Thanks for taking the time to give the detailed and clear explanation. --- Kaartic
Re: BUG: attempt to trim too many characters
Just reminding people that this issue would seem to still exist in
current master..
It's trivial to show:
[torvalds@i7 git]$ git bisect start
[torvalds@i7 git]$ git bisect bad master
[torvalds@i7 git]$ git bisect good master~5
Bisecting: 23 revisions left to test after this (roughly 5 steps)
[f35a1d75b5ecefaef7b1a8ec55543c82883df82f] Merge branch
'rs/t3700-clean-leftover' into maint
[torvalds@i7 git]$ git rev-parse --bisect
fatal: BUG: attempt to trim too many characters
(Note: I use "git rev-parse --bisect" to show it as an error on the
command line, but normal people would obviously do "gitk --bisect"
that then does that "git rev-parse" internally and shows a UI error
box instead).
Linus
On Tue, Sep 5, 2017 at 3:03 PM, Jeff King wrote:
> On Tue, Sep 05, 2017 at 02:55:08PM -0700, Linus Torvalds wrote:
>
>> On Tue, Sep 5, 2017 at 2:50 PM, Jeff King wrote:
>> >
>> > What version of git are you running? This should be fixed by 03df567fbf
>> > (for_each_bisect_ref(): don't trim refnames, 2017-06-18) which is in
>> > v2.14.
>>
>> I'm way more recent than 2.14.
>>
>> I'm at commit 238e487ea ("The fifth batch post 2.14")
>
> Ugh. Bitten again by the fact that rev-parse and revision.c implement
> the same things in subtly different ways.
>
> This probably fixes it:
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 2bd28d3c08..9f24004c0a 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -757,8 +757,8 @@ int cmd_rev_parse(int argc, const char **argv, const char
> *prefix)
> continue;
> }
> if (!strcmp(arg, "--bisect")) {
> - for_each_ref_in("refs/bisect/bad",
> show_reference, NULL);
> - for_each_ref_in("refs/bisect/good",
> anti_reference, NULL);
> + for_each_fullref_in("refs/bisect/bad",
> show_reference, NULL, 0);
> + for_each_fullref_in("refs/bisect/good",
> anti_reference, NULL, 0);
> continue;
> }
> if (opt_with_value(arg, "--branches", &arg)) {
Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.
On Tue, Sep 12, 2017 at 4:30 PM, Kevin Willford wrote: > > Sorry. It was not in the sparse-checkout file. > $ git config core.sparsecheckout true > $ echo /i > .git/info/sparse-checkout > $ git checkout -f > was ran in the modified file case as well and "i" was the only file in the > working directory before reset. > I'm assuming then that you mean that some file "m" is modified by the commit, and do not mean to say that it has local modifications in the working tree? That is not what I had inferred from earlier, so I was very much confused. In this example, the only file actually checked out is "i", as everything else will have the skip-worktree bit set..? So doing git reset HEAD~1 will reset the branch back one commit, and now because of this reset is clearing the skip worktree flag, and since you never had a copy of it checked out it is notified as deleted, because it's no longer marked as skip-worktree? I think the real fix is to stop having reset clear skip-worktree, no? By definition if you do a sparse checkout, you're telling git "I only care about the files in this sparse checkout, and do not concern me with anything else"... So the proposed fix is "since git cleared the skip-worktree flag, we should actually also copy the file out again." but I think the real problem is that we're clearing skip worktree to begin with?
Re: [PATCH] format-patch: use raw format for notes
On Tue, Sep 12, 2017 at 10:33:37AM -0700, Stefan Beller wrote: > On Sun, Sep 10, 2017 at 9:27 PM, Sam Bobroff wrote: > > > (If only there were a way to set the coverletter text automatically as > > well...) > > > > Checkout branch..description AH! I had seen this section of the format-patch man page... --[no-]cover-letter In addition to the patches, generate a cover letter file containing the branch description, shortlog and the overall diffstat. You can fill in a description in the file before sending it out. ... but I didn't realize that it meant that it would insert the text from branch..description into the cover letter. Perhaps there should be a hint to point you to the branch..description section of "git config"? Also, it's not very useful for automating patch submission, as it doesn't set the subject line or remove the "*** BLURB HERE ***" marker, so it must still be post-processed. (To be honest, I was surprised that it didn't use first line as the subject and leave out the markers.) Cheers, Sam.
RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.
> From: Jacob Keller [mailto:jacob.kel...@gmail.com] > Sent: Tuesday, September 12, 2017 4:29 PM > > On Tue, Sep 12, 2017 at 1:20 PM, Kevin Willford wrote: > > > > I think this is where I need to do a better job of explaining so here is a > > simple example. > > > > I have a file "a" that was added in the latest commit. > > $ git log --oneline > > c1fa646 (HEAD -> reset, master) add file a > > 40b342c Initial commit with file i > > > > Running the reset without using a sparse-checkout file > > > > $ git reset HEAD~1 > > $ git status > > On branch reset > > Untracked files: > > (use "git add ..." to include in what will be committed) > > > > a > > > > nothing added to commit but untracked files present (use "git add" to track) > > > > Turning on sparse-checkout and running checkout to make my working > > directory sparse > > > > $ git config core.sparsecheckout true > > $ echo /i > .git/info/sparse-checkout > > $ git checkout -f > > > > Running reset gives me > > $ git reset HEAD~1 > > $ git status > > On branch reset > > nothing to commit, working tree clean > > $ git ls-files > > i > > > > file a is gone. Not in the index and not in the working directory. > > Nothing to let the user know that anything changed. > > > > With a modified file no sparse-checkout > > $ git log --oneline > > 6fbd34a (HEAD -> reset, modified) modified file m > > c734d72 Initial commit with file i and m > > $ git reset HEAD~1 > > Unstaged changes after reset: > > M m > > $ git status > > On branch reset > > Changes not staged for commit: > > (use "git add ..." to update what will be committed) > > (use "git checkout -- ..." to discard changes in working directory) > > > > modified: m > > > > no changes added to commit (use "git add" and/or "git commit -a") > > > > With sparse-checkout > > $ git reset HEAD~1 > > Unstaged changes after reset: > > D m > > $ git status > > On branch reset > > Changes not staged for commit: > > (use "git add/rm ..." to update what will be committed) > > (use "git checkout -- ..." to discard changes in working directory) > > > > deleted:m > > > > no changes added to commit (use "git add" and/or "git commit -a") > > > > Wasn't "m" outside the sparse checkout? Or was it a file in the sparse > checkout? I mean to say, the file after setting up sparse checkout was > one of the "interesting" files that sparse checked out? > > Or was it in fact a separate file which wasn't there? Sorry. It was not in the sparse-checkout file. $ git config core.sparsecheckout true $ echo /i > .git/info/sparse-checkout $ git checkout -f was ran in the modified file case as well and "i" was the only file in the working directory before reset. > > I would think that in sparse-checkout world, you should only *ever* > have the files you list in sparse. > > So files outside sparse world should be ignored, not shown and not > show up in status, but they should absolutely not show up in the > working tree either. Sparse checkout uses the skip-worktree bit which has the following documentation: https://git-scm.com/docs/git-update-index "Skip-worktree bit can be defined in one (long) sentence: When reading an entry, if it is marked as skip-worktree, then Git pretends its working directory version is up to date and read the index version instead. To elaborate, "reading" means checking for file existence, reading file attributes or file content. The working directory version may be present or absent. If present, its content may match against the index version or not. Writing is not affected by this bit, content safety is still first priority. Note that Git can update working directory file, that is marked skip-worktree, if it is safe to do so (i.e. working directory version matches index version)" I'm not saying that is the right behavior, just pointing out the documentation. And from my experience git will turn on and off the skip-worktree flag depending on the command as we see happening with reset. > > You're not "changing" any commits, because the status of the file at > HEAD~1 is exactly what HEAD~1 says it is, but you just don't have a > checked out copy of it. In the case of reset yes and it matches HEAD and if the sparse flag was on, the user does not know that the file was changed in any way, which if I add and commit will create a commit with the file content at HEAD~1 that is outside the sparse area without the user even knowing what those changes were unless after the commit they run a git diff HEAD~1 and see. So files will be changed outside of the sparse area without the user's knowledge. > > I think the key problem is that reset is clearing the sparse flag of a > file so that it no longer shows up as sparse, which is why status > subsequently shows the file deleted (since you don't have a local copy > anymore). And in the case of an added file there is not a sparse flag to clear or keep Because the file didn't exist at HEAD~1 and there is no entry i
[PATCH] doc: fix minor typos (extra/duplicated words)
Following are several fixes for duplicated words ("of of") and one
case where an extra article ("a") slipped in.
Signed-off-by: Evan Zacks
---
Documentation/git-cat-file.txt | 2 +-
Documentation/git-checkout.txt | 2 +-
Documentation/git-notes.txt| 2 +-
Documentation/git-update-index.txt | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 204541c..fb09cd6 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -192,7 +192,7 @@ newline. The available atoms are:
The 40-hex object name of the object.
`objecttype`::
- The type of of the object (the same as `cat-file -t` reports).
+ The type of the object (the same as `cat-file -t` reports).
`objectsize`::
The size, in bytes, of the object (the same as `cat-file -s`
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index d6399c0..bd268a8 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -38,7 +38,7 @@ $ git checkout -b --track /
+
You could omit , in which case the command degenerates to
-"check out the current branch", which is a glorified no-op with a
+"check out the current branch", which is a glorified no-op with
rather expensive side-effects to show only the tracking information,
if exists, for the current branch.
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index be7db30..4367729 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -171,7 +171,7 @@ OPTIONS
object that does not have notes attached to it.
--stdin::
- Also read the object names to remove notes from from the standard
+ Also read the object names to remove notes from the standard
input (there is no reason you cannot combine this with object
names from the command line).
diff --git a/Documentation/git-update-index.txt
b/Documentation/git-update-index.txt
index 1579abf..a14e6ae 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -153,7 +153,7 @@ you will need to handle the situation manually.
+
Version 4 performs a simple pathname compression that reduces index
size by 30%-50% on large repositories, which results in faster load
-time. Version 4 is relatively young (first released in in 1.8.0 in
+time. Version 4 is relatively young (first released in 1.8.0 in
October 2012). Other Git implementations such as JGit and libgit2
may not support it yet.
--
2.2.1
[RFC v2] refs: strip out not allowed flags from ref_transaction_update
Callers are only allowed to pass certain flags into ref_transaction_update, other flags are internal to it. To prevent mistakes from the callers, strip the internal only flags out before continuing. This was noticed because of a compiler warning gcc 7.1.1 issued about passing a NULL parameter as second parameter to memcpy (through hashcpy): In file included from refs.c:5:0: refs.c: In function ‘ref_transaction_verify’: cache.h:948:2: error: argument 2 null where non-null expected [-Werror=nonnull] memcpy(sha_dst, sha_src, GIT_SHA1_RAWSZ); ^~~~ In file included from git-compat-util.h:165:0, from cache.h:4, from refs.c:5: /usr/include/string.h:43:14: note: in a call to function ‘memcpy’ declared here extern void *memcpy (void *__restrict __dest, const void *__restrict __src, ^~ The call to hascpy in ref_transaction_add_update is protected by the passed in flags, but as we only add flags there, gcc notices REF_HAVE_NEW or REF_HAVE_OLD flags could be passed in from the outside, which would potentially result in passing in NULL as second parameter to memcpy. Fix both the compiler warning, and make the interface safer for its users by stripping the internal flags out. Suggested-by: Michael Haggerty Signed-off-by: Thomas Gummerer --- > This might be a nice change to have anyway, to isolate > `ref_transaction_update()` from mistakes by its callers. For that > matter, one might want to be even more selective about what bits are > allowed in the `flags` argument to `ref_transaction_update()`'s > callers: > > > flags &= REF_ALLOWED_FLAGS; /* value would need to be determined */ Here's my attempt at doing this. The odd flag out as the flag that's advertised as internal but can't stripped out is REF_ISPRUNING. REF_ISPRUNING is passed in as argument to 'ref_transaction_delete()' in 'prune_ref()'. Maybe this flag should be public, or maybe I'm missing something here? Having only this internal flags as part of the allowed flags feels a bit ugly, but I'm also unfamiliar with the refs code, hence the RFC. If someone has more suggestions they would be very welcome :) refs.c | 2 ++ refs.h | 8 2 files changed, 10 insertions(+) diff --git a/refs.c b/refs.c index ba22f4acef..fad61be1da 100644 --- a/refs.c +++ b/refs.c @@ -921,6 +921,8 @@ int ref_transaction_update(struct ref_transaction *transaction, return -1; } + flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS; + flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0); ref_transaction_add_update(transaction, refname, flags, diff --git a/refs.h b/refs.h index 6daa78eb50..4d75c207e1 100644 --- a/refs.h +++ b/refs.h @@ -354,6 +354,14 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags); #define REF_NODEREF0x01 #define REF_FORCE_CREATE_REFLOG 0x40 +/* + * Flags that can be passed in to ref_transaction_update + */ +#define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \ + REF_ISPRUNING | \ + REF_FORCE_CREATE_REFLOG |\ + REF_NODEREF + /* * Setup reflog before using. Fill in err and return -1 on failure. */ -- 2.14.1.480.gb18f417b89
Re: [PATCH 4/4] archive: queue directories for all types of pathspecs
Am 20.08.2017 um 11:06 schrieb Jeff King:
> I actually think it would be reasonable to omit the empty directory in
> t5004. The main thing we care about there is that we produce an archive
> with no files rather than barfing. Checking that the empty directory is
> actually there was mostly "this is what it happens to produce" rather
> than any conscious decision, I think.
>
> If the new rule is "we omit empty directories", then it would make sense
> for us to follow that, regardless of whether it happened by pathspec
> limiting or if the tree was empty in the first place (and such an
> empty tree is insane anyway; Git tries hard not to create them).
Right.
-- >8 --
Subject: [PATCH] archive: don't add empty directories to archives
While git doesn't track empty directories, git archive can be tricked
into putting some into archives. One way is to construct an empty tree
object, as t5004 does. While that is supported by the object database,
it can't be represented in the index and thus it's unlikely to occur in
the wild.
Another way is using the literal name of a directory in an exclude
pathspec -- its contents are are excluded, but the directory stub is
included. That's inconsistent: exclude pathspecs containing wildcards
don't leave empty directories in the archive.
Yet another way is have a few levels of nested subdirectories (e.g.
d1/d2/d3/file1) and ignoring the entries at the leaved (e.g. file1).
The directories with the ignored content are ignored as well (e.g. d3),
but their empty parents are included (e.g. d2).
As empty directories are not supported by git, they should also not be
written into archives. If an empty directory is really needed then it
can be tracked and archived by placing an empty .gitignore file in it.
There already is a mechanism in place for suppressing empty directories.
When read_tree_recursive() encounters a directory excluded by a pathspec
then it enters it anyway because it might contain included entries. It
calls the callback function before it is able to decide if the directory
is actually needed. For that reason git archive adds directories to a
queue and writes entries for them only when it encounters the first
child item -- but currently only if pathspecs with wildcards are used.
Queue *all* directories, no matter if there even are pathspecs present.
This prevents git archive from writing entries for empty directories in
all cases.
Suggested-by: Jeff King
Signed-off-by: Rene Scharfe
---
archive.c | 19 ++-
t/t5001-archive-attr.sh | 2 +-
t/t5002-archive-attr-pattern.sh | 2 +-
t/t5004-archive-corner-cases.sh | 4 ++--
4 files changed, 6 insertions(+), 21 deletions(-)
diff --git a/archive.c b/archive.c
index 1ab8d3a1d7..1e41f4bbeb 100644
--- a/archive.c
+++ b/archive.c
@@ -121,11 +121,6 @@ static int check_attr_export_subst(const struct attr_check
*check)
return check && ATTR_TRUE(check->items[1].value);
}
-static int should_queue_directories(const struct archiver_args *args)
-{
- return args->pathspec.has_wildcard;
-}
-
static int write_archive_entry(const unsigned char *sha1, const char *base,
int baselen, const char *filename, unsigned mode, int stage,
void *context)
@@ -147,7 +142,7 @@ static int write_archive_entry(const unsigned char *sha1,
const char *base,
strbuf_addch(&path, '/');
path_without_prefix = path.buf + args->baselen;
- if (!S_ISDIR(mode) || !should_queue_directories(args)) {
+ if (!S_ISDIR(mode)) {
const struct attr_check *check;
check = get_archive_attrs(path_without_prefix);
if (check_attr_export_ignore(check))
@@ -169,14 +164,6 @@ static int write_archive_entry(const unsigned char *sha1,
const char *base,
return write_entry(args, sha1, path.buf, path.len, mode);
}
-static int write_archive_entry_buf(const unsigned char *sha1, struct strbuf
*base,
- const char *filename, unsigned mode, int stage,
- void *context)
-{
- return write_archive_entry(sha1, base->buf, base->len,
-filename, mode, stage, context);
-}
-
static void queue_directory(const unsigned char *sha1,
struct strbuf *base, const char *filename,
unsigned mode, int stage, struct archiver_context *c)
@@ -290,9 +277,7 @@ int write_archive_entries(struct archiver_args *args,
}
err = read_tree_recursive(args->tree, "", 0, 0, &args->pathspec,
- should_queue_directories(args) ?
- queue_or_write_archive_entry :
- write_archive_entry_buf,
+ queue_or_write_archive_entry,
&context);
if (err == READ_TREE_RECURSIVE)
err = 0;
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
Re: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.
On Tue, Sep 12, 2017 at 1:20 PM, Kevin Willford wrote: >> From: Junio C Hamano [mailto:gits...@pobox.com] >> Sent: Monday, September 11, 2017 9:57 PM >> >> Let's imagine a path P that is outside the sparse checkout area. >> And we check out a commit that has P. P would still be recorded in >> the index but it would not materialize in the working tree. "git >> status" and friends are asked not to treat this as "locally removed", >> to prevent "commit -a" from recording a removal, of course. >> >> Now, let's further imagine that you have a checkout of the same >> project but at a commit that does not have P. Then you reset to >> another commit that does have P. My understanding of what Kevin's >> first test wants to demonstrate is that the index is populated with >> P (because you did reset to a commit with that path) but it does not >> materialize in the working tree (perhaps because that is outside the >> sparse checkout area?), yet there is something missing compared to >> the earlier case where "git status" and friends are asked not to >> treat P as "locally removed". They instead show P as locally removed, >> and "commit -a" would record a removal---that is indeed a problem. >> >> Am I reading the problem description correctly so far? If so, then >> my answer to my first question (are we solving a right problem?) is >> yes. >> > > I think this is where I need to do a better job of explaining so here is a > simple example. > > I have a file "a" that was added in the latest commit. > $ git log --oneline > c1fa646 (HEAD -> reset, master) add file a > 40b342c Initial commit with file i > > Running the reset without using a sparse-checkout file > > $ git reset HEAD~1 > $ git status > On branch reset > Untracked files: > (use "git add ..." to include in what will be committed) > > a > > nothing added to commit but untracked files present (use "git add" to track) > > Turning on sparse-checkout and running checkout to make my working > directory sparse > > $ git config core.sparsecheckout true > $ echo /i > .git/info/sparse-checkout > $ git checkout -f > > Running reset gives me > $ git reset HEAD~1 > $ git status > On branch reset > nothing to commit, working tree clean > $ git ls-files > i > > file a is gone. Not in the index and not in the working directory. > Nothing to let the user know that anything changed. > > With a modified file no sparse-checkout > $ git log --oneline > 6fbd34a (HEAD -> reset, modified) modified file m > c734d72 Initial commit with file i and m > $ git reset HEAD~1 > Unstaged changes after reset: > M m > $ git status > On branch reset > Changes not staged for commit: > (use "git add ..." to update what will be committed) > (use "git checkout -- ..." to discard changes in working directory) > > modified: m > > no changes added to commit (use "git add" and/or "git commit -a") > > With sparse-checkout > $ git reset HEAD~1 > Unstaged changes after reset: > D m > $ git status > On branch reset > Changes not staged for commit: > (use "git add/rm ..." to update what will be committed) > (use "git checkout -- ..." to discard changes in working directory) > > deleted:m > > no changes added to commit (use "git add" and/or "git commit -a") > Wasn't "m" outside the sparse checkout? Or was it a file in the sparse checkout? I mean to say, the file after setting up sparse checkout was one of the "interesting" files that sparse checked out? Or was it in fact a separate file which wasn't there? I would think that in sparse-checkout world, you should only *ever* have the files you list in sparse. So files outside sparse world should be ignored, not shown and not show up in status, but they should absolutely not show up in the working tree either. You're not "changing" any commits, because the status of the file at HEAD~1 is exactly what HEAD~1 says it is, but you just don't have a checked out copy of it. I think the key problem is that reset is clearing the sparse flag of a file so that it no longer shows up as sparse, which is why status subsequently shows the file deleted (since you don't have a local copy anymore). Am I understanding the problem correctly? I think your examples above are not clear because they don't seem to be each complete individually (The sparse checkout examples should start from a clean world and explain how they got there rather than relying on imformation in the prior non sparse example. We should be clear so everyone knows what the problem is). If you're performing a sparse checkout and you modify files outside of the sparse area, I think that will cause problems, and we may not be making the best efforts to resolve it. I do agree that if you have created a file "m" when only "i" is in your path, that we should absolutely not delete "m" but leave it as is. Thanks, Jake > I think we can agree that this is not the correct behavior. > >> But this time, I am trying to see if the approach is good. I am not >>
RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Monday, September 11, 2017 9:57 PM > > Let's imagine a path P that is outside the sparse checkout area. > And we check out a commit that has P. P would still be recorded in > the index but it would not materialize in the working tree. "git > status" and friends are asked not to treat this as "locally removed", > to prevent "commit -a" from recording a removal, of course. > > Now, let's further imagine that you have a checkout of the same > project but at a commit that does not have P. Then you reset to > another commit that does have P. My understanding of what Kevin's > first test wants to demonstrate is that the index is populated with > P (because you did reset to a commit with that path) but it does not > materialize in the working tree (perhaps because that is outside the > sparse checkout area?), yet there is something missing compared to > the earlier case where "git status" and friends are asked not to > treat P as "locally removed". They instead show P as locally removed, > and "commit -a" would record a removal---that is indeed a problem. > > Am I reading the problem description correctly so far? If so, then > my answer to my first question (are we solving a right problem?) is > yes. > I think this is where I need to do a better job of explaining so here is a simple example. I have a file "a" that was added in the latest commit. $ git log --oneline c1fa646 (HEAD -> reset, master) add file a 40b342c Initial commit with file i Running the reset without using a sparse-checkout file $ git reset HEAD~1 $ git status On branch reset Untracked files: (use "git add ..." to include in what will be committed) a nothing added to commit but untracked files present (use "git add" to track) Turning on sparse-checkout and running checkout to make my working directory sparse $ git config core.sparsecheckout true $ echo /i > .git/info/sparse-checkout $ git checkout -f Running reset gives me $ git reset HEAD~1 $ git status On branch reset nothing to commit, working tree clean $ git ls-files i file a is gone. Not in the index and not in the working directory. Nothing to let the user know that anything changed. With a modified file no sparse-checkout $ git log --oneline 6fbd34a (HEAD -> reset, modified) modified file m c734d72 Initial commit with file i and m $ git reset HEAD~1 Unstaged changes after reset: M m $ git status On branch reset Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) modified: m no changes added to commit (use "git add" and/or "git commit -a") With sparse-checkout $ git reset HEAD~1 Unstaged changes after reset: D m $ git status On branch reset Changes not staged for commit: (use "git add/rm ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) deleted:m no changes added to commit (use "git add" and/or "git commit -a") I think we can agree that this is not the correct behavior. > But this time, I am trying to see if the approach is good. I am not > so sure if the approach taken by this patch is so obviously good as > you seem to think. A logical consequence of the approach "git > status thinks that P appears in the index and missing in the working > tree is a local removal, so let's squelch it by creating the file in > the working tree" is that we will end up fully populating the > working tree with paths that are clearly outside the area the user > designated as the sparse checkout area---surely that may squelch > "status", but to me, it looks like it is breaking what the user > wanted to achieve with "sparse checkout" in the first place. > I don't think that we are trying to "squelch" status so much as make it consistent with what the user would expect to happen. If that means not resetting entries with the skip-worktree bit or resetting the entries but keeping the skip-worktree bit on, okay, but I would argue that is not what the user wants because if you are now saying that sparse means git will not change files outside the sparse-checkout entries, what about merge, rebase, cherry-pick, apply? Should those only change the files that are in the sparse definition? If so we would be changing the commits from the original, i.e. cherry-pick 123 would create a different commit depending on whether or not you are using sparse as well as a different commit depending on what is in your sparse-checkout. I see reset being a similar scenario in that if everything is clean, after I "reset HEAD~1" I should be able to run "add ." + "commit" and have the same commit as before the reset. If this is changed to only reset the sparse entries, there will be staged changes after the reset because HEAD has changed but we didn't update the index versions of the files. If we do update the index with the "HEAD~1"
GERARD SANCHEZ
"My name is Gerard Sanchez , a real estate agent presently based in UAE. I have been diagnosed with Oesophageal cancer. I have been on medical treatment after which it was confirmed by my doctor that I have cancer in 2012. The sad part now is that it has also been confirmed that i may not live to see next year for this reason i have decided to share my wealth to people who can make better use of it while i am gone. I want to transfer the sum of 3 Million Dollars to you.I hope and believe that God directed me to the right person that will use these funds to fund orphanage homes,help the needy and widows. I have always been a God fearing man and i always love to help the poor. I want to extend this help to the people outside my region sadly my present health situation will not allow me move hence i have decided by the help of God to get this money to you and expect you to use some for your personal use and the others for charity work. I have lodge this money in a consignment please confirm your interest so i can tell you more on how to get this money. Please provide below informations to begin claims formalities Full Names: Address: Contact no: Date of Birth: Country of Residence Ensure to send the above informations to my personal email address. Please take note ( gerrard20139sanc...@gmail.com) I believe so much in God and my faith has lead me to you please reply as soon as possible. Warm Regards. MR GERARD SANCHEZ
[no subject]
<>
Re: commit-msg hook does not run on merge with --no-ff option
Sorry I don't understand your question. The commit-msg hook runs properly in all cases except when I perform a merge with the --no-ff option enabled. On Mon, Sep 11, 2017 at 12:25 PM, Stefan Beller wrote: > On Mon, Sep 11, 2017 at 7:34 AM, Joseph Dunne wrote: > >> When I merge ... however my commit-msg hook does not run. (The >> commit-msg hook works fine in all other commits / merges.) > > When using git-commit, but not git-merge? > > See the discussion of patches at > https://public-inbox.org/git/20170907220429.31312-1-sbel...@google.com/
Re: SHA1 collision in production repo?! (probably not)
On Tue, Sep 12, 2017 at 06:18:32PM +0200, Lars Schneider wrote: > we are seeing this now in Git 2.14.1: > > ... > error: inflate: data stream error (unknown compression method) > error: unable to unpack 7b513f98a66ef9488e516e7abbc246438597c6d5 header > error: inflate: data stream error (unknown compression method) > error: unable to unpack 7b513f98a66ef9488e516e7abbc246438597c6d5 header > fatal: loose object 7b513f98a66ef9488e516e7abbc246438597c6d5 (stored in > .git/objects/7b/513f98a66ef9488e516e7abbc246438597c6d5) is corrupt > fatal: The remote end hung up unexpectedly > > I guess this means your fix [1] works properly :-) Oh, good. :) > At some point I will try to explore a retry mechanism for these cases. I don't think we can generally retry loose-object failures. We use copies from packs first, and then loose. So a corrupt loose can fallback to another pack or to loose, but not the other way around (because we would never look at the loose if we had a good copy elsewhere). Though in your particular case, if I recall, you're receiving the object over the network and the corrupted copy is in the way. So right now the recovery process is: 1. Notice the commit message. 2. Run git-fsck to notice that we don't really[1] need the object. 3. Run `rm .git/objects/7b/513f...` 4. Re-run the fetch. But in theory we should be able to say "oh, we don't _really_ have that, the collision test isn't necessary" and then overwrite it. I actually thought that's what would happen now (has_sha1_file() would return an error), but I guess for what we need, it just does a stat() and calls it a day, not realizing we ought to be overwriting. -Peff [1] git-fsck will actually complain if reflogs point to the object, and we can always expire those in a corrupted repo. So possibly what you want to know is whether it's reachable from actual refs. Of course this whole check is optional. If the object's corrupted, it's corrupted. But I get nervous calling `rm` on something that _could_ be precious (say it's just a single-bit error that could be recovered). But if you have a known-good copy incoming, that's less of an issue.
Re: [PATCH] format-patch: use raw format for notes
On Sun, Sep 10, 2017 at 9:27 PM, Sam Bobroff wrote: > (If only there were a way to set the coverletter text automatically as > well...) > Checkout branch..description
[PATCH 4/4] packed refs: pass .git dir instead of packed-refs path to init_fn
From: Stefan Beller
The first argument of a ref_store_init_fn is documented to represent
the $GIT_DIR, not the path to the packed-refs file. This brings the
packed-refs store more in line with the usual ref store interface.
Signed-off-by: Jonathan Nieder
Signed-off-by: Stefan Beller
---
That's the end of the series. Thanks for reading.
refs/files-backend.c | 4 ++--
refs/packed-backend.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index fccbc24ac4..3b8e13a8b7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -57,8 +57,8 @@ static struct ref_store *files_ref_store_create(const char
*gitdir,
refs->gitdir = xstrdup(gitdir);
get_common_dir_noenv(&sb, gitdir);
refs->gitcommondir = strbuf_detach(&sb, NULL);
- strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
- refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
+ refs->packed_ref_store =
+ packed_ref_store_create(refs->gitcommondir, flags);
strbuf_release(&sb);
return ref_store;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 412c85034f..2c5db15279 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -78,7 +78,7 @@ struct packed_ref_store {
struct tempfile tempfile;
};
-struct ref_store *packed_ref_store_create(const char *path,
+struct ref_store *packed_ref_store_create(const char *common_dir,
unsigned int store_flags)
{
struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
@@ -87,7 +87,7 @@ struct ref_store *packed_ref_store_create(const char *path,
base_ref_store_init(ref_store, &refs_be_packed);
refs->store_flags = store_flags;
- refs->path = xstrdup(path);
+ refs->path = xstrfmt("%s/packed-refs", common_dir);
return ref_store;
}
--
2.14.1.690.gbb1197296e
[PATCH 3/4] replace-objects: evaluate replacement refs without using the object store
From: Stefan Beller Pass DO_FOR_EACH_INCLUDE_BROKEN when iterating over replacement refs so that the iteration does not require opening the named objects from the object store. This avoids a dependency cycle between object access and replace ref iteration. Moreover the ref subsystem has not been migrated yet to access the object store via passed in repository objects. As a result, without this patch, iterating over replace refs in a repository other than the_repository it produces errors: error: refs/replace/3afabef75c627b8943bcae86837abc7c32fe does not point to a valid object! Noticed while adapting the object store (and in particular its evaluation of replace refs) to handle arbitrary repositories. Signed-off-by: Stefan Beller Signed-off-by: Jonathan Nieder --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index b0106b8162..cd84ed9710 100644 --- a/refs.c +++ b/refs.c @@ -1394,7 +1394,7 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data) return do_for_each_ref(get_main_ref_store(), git_replace_ref_base, fn, strlen(git_replace_ref_base), - 0, cb_data); + DO_FOR_EACH_INCLUDE_BROKEN, cb_data); } int for_each_namespaced_ref(each_ref_fn fn, void *cb_data) -- 2.14.1.690.gbb1197296e
[PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits
From: Stefan Beller
The check_has_commit helper uses resolves a submodule entry to a
commit, when validating its existence. As a side effect this means
tolerates a submodule entry pointing to a tag, which is not a valid
submodule entry that git commands would know how to cope with.
Tighten the check to require an actual commit, not a tag pointing to a
commit.
Also improve the error handling when a submodule entry points to
non-commit (e.g., a blob) to error out instead of warning and
pretending the pointed to object doesn't exist.
Signed-off-by: Stefan Beller
Signed-off-by: Jonathan Nieder
---
submodule.c| 33 +
t/t5531-deep-submodule-push.sh | 10 ++
2 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/submodule.c b/submodule.c
index 3cea8221e0..e0da55920d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -767,19 +767,36 @@ static int append_oid_to_argv(const struct object_id
*oid, void *data)
return 0;
}
+struct has_commit_data {
+ int result;
+ const char *path;
+};
+
static int check_has_commit(const struct object_id *oid, void *data)
{
- int *has_commit = data;
+ struct has_commit_data *cb = data;
- if (!lookup_commit_reference(oid))
- *has_commit = 0;
+ enum object_type type = sha1_object_info(oid->hash, NULL);
- return 0;
+ switch (type) {
+ case OBJ_COMMIT:
+ return 0;
+ case OBJ_BAD:
+ /*
+* Object is missing or invalid. If invalid, an error message
+* has already been printed.
+*/
+ cb->result = 0;
+ return 0;
+ default:
+ die(_("submodule entry '%s' (%s) is a %s, not a commit"),
+ cb->path, oid_to_hex(oid), typename(type));
+ }
}
static int submodule_has_commits(const char *path, struct oid_array *commits)
{
- int has_commit = 1;
+ struct has_commit_data has_commit = { 1, path };
/*
* Perform a cheap, but incorrect check for the existence of 'commits'.
@@ -795,7 +812,7 @@ static int submodule_has_commits(const char *path, struct
oid_array *commits)
oid_array_for_each_unique(commits, check_has_commit, &has_commit);
- if (has_commit) {
+ if (has_commit.result) {
/*
* Even if the submodule is checked out and the commit is
* present, make sure it exists in the submodule's object store
@@ -814,12 +831,12 @@ static int submodule_has_commits(const char *path, struct
oid_array *commits)
cp.dir = path;
if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)
- has_commit = 0;
+ has_commit.result = 0;
strbuf_release(&out);
}
- return has_commit;
+ return has_commit.result;
}
static int submodule_needs_pushing(const char *path, struct oid_array *commits)
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 0f84a53146..39cb2c1c34 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -298,6 +298,16 @@ test_expect_success 'push succeeds if submodule commit
disabling recursion from
)
'
+test_expect_success 'submodule entry pointing at a tag is error' '
+ git -C work/gar/bage tag -a test1 -m "tag" &&
+ tag=$(git -C work/gar/bage rev-parse test1^{tag}) &&
+ git -C work update-index --cacheinfo 16 "$tag" gar/bage &&
+ git -C work commit -m "bad commit" &&
+ test_when_finished "git -C work reset --hard HEAD^" &&
+ test_must_fail git -C work push --recurse-submodules=on-demand
../pub.git master 2>err &&
+ test_i18ngrep "is a tag, not a commit" err
+'
+
test_expect_success 'push fails if recurse submodules option passed as yes' '
(
cd work/gar/bage &&
--
2.14.1.690.gbb1197296e
[PATCH 1/4] pack: make packed_git_mru global a value instead of a pointer
The MRU cache that keeps track of recently used packs is represented
using two global variables:
struct mru packed_git_mru_storage;
struct mru *packed_git_mru = &packed_git_mru_storage;
Callers never assign to the packed_git_mru pointer, though, so we can
simplify by eliminating it and using &packed_git_mru_storage (renamed
to &packed_git_mru) directly. This variable is only used by the
packfile subsystem, making this a relatively uninvasive change (and
any new unadapted callers would trigger a compile error).
Noticed while moving these globals to the object_store struct.
Signed-off-by: Stefan Beller
Signed-off-by: Jonathan Nieder
Acked-by: Jeff King
---
Unchanged from
https://public-inbox.org/git/20170830064827.gb153...@aiede.mtv.corp.google.com/.
I agree with the comments there that it would be nice to make this use
list.h (#leftoverbits) and would be happy to read a patch doing that.
Once we're done adapting the object store to work with arbitrary
repositories we may come back to this, but I'd be happier if it's just
already taken care of for us.
builtin/pack-objects.c | 4 ++--
cache.h| 4 ++--
packfile.c | 12 +---
3 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a57b4f058d..f721137eaf 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1012,7 +1012,7 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
- for (entry = packed_git_mru->head; entry; entry = entry->next) {
+ for (entry = packed_git_mru.head; entry; entry = entry->next) {
struct packed_git *p = entry->item;
off_t offset;
@@ -1030,7 +1030,7 @@ static int want_object_in_pack(const unsigned char *sha1,
}
want = want_found_object(exclude, p);
if (!exclude && want > 0)
- mru_mark(packed_git_mru, entry);
+ mru_mark(&packed_git_mru, entry);
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index a916bc79e3..49b083ee0a 100644
--- a/cache.h
+++ b/cache.h
@@ -4,6 +4,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
#include "hashmap.h"
+#include "mru.h"
#include "advice.h"
#include "gettext.h"
#include "convert.h"
@@ -1589,8 +1590,7 @@ extern struct packed_git {
* A most-recently-used ordered version of the packed_git list, which can
* be iterated instead of packed_git (and marked via mru_mark).
*/
-struct mru;
-extern struct mru *packed_git_mru;
+extern struct mru packed_git_mru;
struct pack_entry {
off_t offset;
diff --git a/packfile.c b/packfile.c
index f86fa051c9..f69a5c8d60 100644
--- a/packfile.c
+++ b/packfile.c
@@ -40,9 +40,7 @@ static unsigned int pack_max_fds;
static size_t peak_pack_mapped;
static size_t pack_mapped;
struct packed_git *packed_git;
-
-static struct mru packed_git_mru_storage;
-struct mru *packed_git_mru = &packed_git_mru_storage;
+struct mru packed_git_mru;
#define SZ_FMT PRIuMAX
static inline uintmax_t sz_fmt(size_t s) { return s; }
@@ -861,9 +859,9 @@ static void prepare_packed_git_mru(void)
{
struct packed_git *p;
- mru_clear(packed_git_mru);
+ mru_clear(&packed_git_mru);
for (p = packed_git; p; p = p->next)
- mru_append(packed_git_mru, p);
+ mru_append(&packed_git_mru, p);
}
static int prepare_packed_git_run_once = 0;
@@ -1832,9 +1830,9 @@ int find_pack_entry(const unsigned char *sha1, struct
pack_entry *e)
if (!packed_git)
return 0;
- for (p = packed_git_mru->head; p; p = p->next) {
+ for (p = packed_git_mru.head; p; p = p->next) {
if (fill_pack_entry(sha1, e, p->item)) {
- mru_mark(packed_git_mru, p);
+ mru_mark(&packed_git_mru, p);
return 1;
}
}
--
2.14.1.690.gbb1197296e
[PATCH 0/4] Fixes from the per-repository object store series
Hi, This is a continuation of the series at [1]. That was part 1 --- you can think of this as part 0, since it contains the simplest and least controversial part of the series --- each patch in this series is a bugfix in its own right. Patch 1 should be familiar if you reviewed the series [1]. It is unchanged from the patch there, except to note Peff's ack. Patches 2-4 have not yet visited the list but are fixes (or, in the case of patch 4, cleanups) noticed as part of the same process of teaching object store code to handle multiple repositories. We hope that splitting these out should make them easier to review and for people to benefit from these fixes without having to wait for the rest of the series to settle. Thoughts of all kinds welcome. Jonathan Nieder (1): pack: make packed_git_mru global a value instead of a pointer Stefan Beller (3): push, fetch: error out for submodule entries not pointing to commits replace-objects: evaluate replacement refs without using the object store packed refs: pass .git dir instead of packed-refs path to init_fn builtin/pack-objects.c | 4 ++-- cache.h| 4 ++-- packfile.c | 12 +--- refs.c | 2 +- refs/files-backend.c | 4 ++-- refs/packed-backend.c | 4 ++-- submodule.c| 33 + t/t5531-deep-submodule-push.sh | 10 ++ 8 files changed, 49 insertions(+), 24 deletions(-) [1] https://public-inbox.org/git/20170830064634.ga153...@aiede.mtv.corp.google.com/
Re: SHA1 collision in production repo?! (probably not)
> On 31 Mar 2017, at 19:45, Jeff King wrote: > > On Fri, Mar 31, 2017 at 10:35:06AM -0700, Junio C Hamano wrote: > >> Lars Schneider writes: >> >>> Hi, >>> >>> I just got a report with the following output after a "git fetch" operation >>> using Git 2.11.0.windows.3 [1]: >>> >>> remote: Counting objects: 5922, done. >>> remote: Compressing objects: 100% (14/14), done. >>> error: inflate: data stream error (unknown compression method) >>> error: unable to unpack 6acd8f279a8b20311665f41134579b7380970446 header >>> fatal: SHA1 COLLISION FOUND WITH 6acd8f279a8b20311665f41134579b7380970446 ! >>> fatal: index-pack failed >>> >>> I would be really surprised if we discovered a SHA1 collision in a >>> production >>> repo. My guess is that this is somehow triggered by a network issue (see >>> data >>> stream error). Any tips how to debug this? >> >> Perhaps the first thing to do is to tweak the messages in >> builtin/index-pack.c >> to help you identify which one of identical 5 messages is firing. >> >> My guess would be that the code saw an object that came over the >> wire, hashed it to learn that its object name is >> 6acd8f279a8b20311665f41134579b7380970446, noticed that it locally >> already has the object with the same name, and tried to make sure >> they have identical contents (otherwise, what came over the wire is >> a successful second preimage attack). But your loose object on disk >> you already had was corrupt and didn't inflate correctly when >> builtin/index-pack.c::compare_objects() or check_collision() tried >> to. The code saw no data, or truncated data, or >> whatever---something different from what the other data that hashed >> down to 6acd8..., and reported a collision when there is no >> collision. > > My guess is the one in compare_objects(). The "unable to unpack" error > comes from sha1_loose_object_info(). We'd normally then follow up with > read_sha1_file(), which would generate its own set of errors. > > But if open_istream() got a bogus value for the object size (but didn't > correctly report an error), then it would probably quietly return 0 > bytes from read_istream() later. > > I suspect this may improve things, but I haven't dug deeper to see if > there are unwanted side effects, or if there are other spots that need > similar treatment. > > diff --git a/sha1_file.c b/sha1_file.c > index 43990dec7..38411f90b 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -2952,7 +2952,7 @@ static int sha1_loose_object_info(const unsigned char > *sha1, > if (status && oi->typep) > *oi->typep = status; > strbuf_release(&hdrbuf); > - return 0; > + return status; > } > > int sha1_object_info_extended(const unsigned char *sha1, struct object_info > *oi, unsigned flags) Hi Peff, we are seeing this now in Git 2.14.1: ... error: inflate: data stream error (unknown compression method) error: unable to unpack 7b513f98a66ef9488e516e7abbc246438597c6d5 header error: inflate: data stream error (unknown compression method) error: unable to unpack 7b513f98a66ef9488e516e7abbc246438597c6d5 header fatal: loose object 7b513f98a66ef9488e516e7abbc246438597c6d5 (stored in .git/objects/7b/513f98a66ef9488e516e7abbc246438597c6d5) is corrupt fatal: The remote end hung up unexpectedly I guess this means your fix [1] works properly :-) At some point I will try to explore a retry mechanism for these cases. Cheers, Lars [1] https://github.com/git/git/commit/93cff9a978e1c177ac3e889867004a56773301b2
Re: Buffered value should be shown when requesting username for remote authentication
On Tue, Sep 12, 2017 at 06:17:29PM +0530, Kaartic Sivaraam wrote: > I noted a little issue with the interaction to a remote git repository. > The issue occurs when the network used for remote communication is a > bit sluggish. The main issue is illustrated by the following shell > interaction, > > $ git push -u fork > sivaraamUsername for 'https://github.com': sivaraam > Password for 'https://sivaraamsivar...@github.com': > > It's a little odd that the (buffered) input found before the request > for 'Username' is also appended to the input found after the request. > This might not be obvious to the user and he has to retype the > 'Username' in the next try. If I understand right, you typed "sivaraam" once, then the network lagged, then you typed "sivaraam" again. That isn't really something that Git can fix reliably. Reading those characters and echoing them back to the terminal is handled by your terminal driver (and potentially other things like ssh). Git may have received "sivaraamsivaraam" all at once, depending on where the lag is. -Peff
Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives
On Tue, Sep 12, 2017 at 07:11:38PM +0530, Kaartic Sivaraam wrote: > On Tue, 2017-09-05 at 09:05 -0400, Jeff King wrote: > > This patch introduces an UNLEAK() macro that lets us do so. > > To understand its design, let's first look at some of the > > alternatives. > > > > > This patch adds the UNLEAK() macro and enables it > > automatically when Git is compiled with SANITIZE=leak. > > It adds some UNLEAK() annotations to show off how the > > feature works. On top of other recent leak fixes, these are > > enough to get t and t0001 to pass when compiled with > > LSAN. > > My nit of the day ;-) > > The above paragraphs seems to going against the following guideline of > Documentation/SubmittingPatches, > > Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behavior. Try to make sure your explanation can be understood Like all good writing rules, I think it's important to know when to break them. :) Writing in the imperative is _most_ important in the subject. You're likely to see a lot of subjects in a list, and it makes the list easier to read if they all match. It also tends to be shorter, which is good for subjects. For short commit messages, I think the imperative also keeps things tight and to the point: describe the problem and then say how to fix it. The recent 0db3dc75f is a good example (which I picked by skimming recent "git log" output). But saying "this patch" is IMHO not that big a problem there, as long as it isn't done excessively. When you the explanation is longer or more complicated, the imperative can actually be a bit _too_ terse. In longer text it helps to guide readers in the direction you want their thoughts to take. Having a three-paragraph explanation of the problem or current state of things and then jumping right into "Do this. Do that." lacks context. A marker like "this patch" helps the reader know that you're switching gears to talking about the solution. I also think that "I" is useful in avoiding the passive voice. It can certainly be used gratuitously and make things less clear, but in most cases I'd rather see something like "I tested performance under these conditions" than "Performance was tested under these conditions". I also often use the "academic we" here even when I worked on something myself. -Peff
Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives
On Tue, Sep 12, 2017 at 08:04:52PM +0530, Kaartic Sivaraam wrote: > > On Tue, 2017-09-05 at 15:05 -0700, Stefan Beller wrote: > > > > After having a sneak peak at the implementation > > it is O(1) in runtime for each added element, and the > > space complexity is O(well). > > > > Incidentally I was reading about "complexity of algorithms" and there > was the following paragraph in the book, > > > Unfortunately, as Knuth observed, big-O notation is often used by > careless writers and > speakers as if it had the same meaning as big-Theta notation. Keep this > in mind when you see > big-O notation used. The recent trend has been to use big-Theta notation > whenever both upper > and lower bounds on the size of a function are needed. > > So, if my interpretation is correct the above usage of O(1) and O(well) > should have been Θ(1) and Θ(well). But theta-well isn't a pun. :P It is true that prepending to a linked list is also Θ(1), but I'm not sure if it's carelessness that causes many programmers to use big-O. It's that what we care about is worst-case performance. So knowing that we have a lower bound isn't usually that interesting. What we want to know is whether it will blow up in our face as "n" gets large. Plus typing non-ascii characters is annoying. :) If you want to talk about sloppy analysis, the two most common problems I see are: 1. People talk about big-O complexity without discussing constants. For reasonable values of "n", the constants often matter (they're not wrong about big-O, but they are wrong about what will run fast in practice). 2. Glossing over things like amortized costs. Hash tables aren't really O(1) because they eventually fill up and get collisions. You have to talk about load factor, resizing, etc. I'm sure I'm guilty of doing those things sometimes, too. -Peff
Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives
> On Tue, 2017-09-05 at 15:05 -0700, Stefan Beller wrote: > > After having a sneak peak at the implementation > it is O(1) in runtime for each added element, and the > space complexity is O(well). > Incidentally I was reading about "complexity of algorithms" and there was the following paragraph in the book, Unfortunately, as Knuth observed, big-O notation is often used by careless writers and speakers as if it had the same meaning as big-Theta notation. Keep this in mind when you see big-O notation used. The recent trend has been to use big-Theta notation whenever both upper and lower bounds on the size of a function are needed. So, if my interpretation is correct the above usage of O(1) and O(well) should have been Θ(1) and Θ(well). -- Kaartic
Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives
On Tue, 2017-09-05 at 09:05 -0400, Jeff King wrote: > This patch introduces an UNLEAK() macro that lets us do so. > To understand its design, let's first look at some of the > alternatives. > > This patch adds the UNLEAK() macro and enables it > automatically when Git is compiled with SANITIZE=leak. > It adds some UNLEAK() annotations to show off how the > feature works. On top of other recent leak fixes, these are > enough to get t and t0001 to pass when compiled with > LSAN. My nit of the day ;-) The above paragraphs seems to going against the following guideline of Documentation/SubmittingPatches, Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behavior. Try to make sure your explanation can be understood -- Kaartic
Re: Unexpected pass for t6120-describe.sh on cygwin
Hi Ramsay, On Sat, 9 Sep 2017, Ramsay Jones wrote: > I ran the test-suite on the 'pu' branch last night (simply because that > was what I had built at the time!), which resulted in a PASS, but t6120 > was showing a 'TODO passed' for #52. > > This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack' > branch, which uses 'ulimit -s' to try and force a stack overflow. > Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created > a test program (see below) to eat up the stack and tried running it with > various ulimit values (128, 12, 8), but it always seg-faulted at the > same stack-frame. (after using approx 2MB stack space). > > So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled > on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests, but > haven't looked into it. > > Given that 'ulimit' is a bash built-in, this may also be a problem on > MinGW and Git-For-Windows, but I can't test on those platforms. It is. Thanks, Dscho
Buffered value should be shown when requesting username for remote authentication
Hello all, I noted a little issue with the interaction to a remote git repository. The issue occurs when the network used for remote communication is a bit sluggish. The main issue is illustrated by the following shell interaction, $ git push -u fork sivaraamUsername for 'https://github.com': sivaraam Password for 'https://sivaraamsivar...@github.com': It's a little odd that the (buffered) input found before the request for 'Username' is also appended to the input found after the request. This might not be obvious to the user and he has to retype the 'Username' in the next try. I have seen some utilities handle this correctly by doing something like, $ sivaUsername for 'https://github.com': siva Further, unlike the request string, the buffered input that gets printed after the request was editable. I'm not able to recollect the correctly, though. Any ideas about how to fix this? -- Kaartic
Re: [PATCH v2] sub-process: print the cmd when a capability is unsupported
> On 11 Sep 2017, at 05:27, Junio C Hamano wrote:
>
> Junio C Hamano writes:
>
>> I still think we would want to turn warning() to die(), but it
>> probably is better to do so in a separate follow-up patch. That
>> will give us a good place to record the reason why the current "just
>> call a warning() and pretend as if nothing bad happend" is wrong.
>
> And here is such an update. It seems that pretty much all comments
> in the original thread were "warning is wrong--we should die here",
> but nobody seems to have bothered following it through.
>
> cf. <20170815111725.5d009...@twelve2.svl.corp.google.com>
>
> -- >8 --
> Subject: [PATCH] subprocess: loudly die when subprocess asks for an
> unsupported capability
>
> The handshake_capabilities() function first advertises the set of
> capabilities it supports, so that the other side can pick and choose
> which ones to use and ask us to enable in its response. Then we
> read the response that tells us what choice the other side made. If
> we saw something that we never advertised, that indicates one of two
> things. The other side, i.e. the "upgraded" filter, is not paying
> attention of the capabilities advertisement, and asking something
> its correct operation relies on, but we are not capable of giving
> that unknown feature and operate without it, so after that point the
> exchange of data is a garbage-in-garbage-out. Or the other side
> wanted to ask for one of the capabilities we advertised, but the
> code has typo and their wish to enable a capability that its correct
> operation relies on is not understood on this end. The result is
> the same garbage-in-garbage-out.
>
> Instead of sweeping such a potential bug under the rug, die loudly
> when we see a request for an unsupported capability in order to
> force sloppily-written filter scripts to get corrected.
>
> Signed-off-by: Junio C Hamano
> ---
> sub-process.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sub-process.c b/sub-process.c
> index fcc4832c14..ec9a51b7b1 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -181,8 +181,8 @@ static int handshake_capabilities(struct child_process
> *process,
> if (supported_capabilities)
> *supported_capabilities |= capabilities[i].flag;
> } else {
> - warning("subprocess '%s' requested unsupported
> capability '%s'",
> - process->argv[0], p);
> + die("subprocess '%s' requested unsupported capability
> '%s'",
> + process->argv[0], p);
Looks good!
Thank you,
Lars
Re: [PATCH 00/12] Clean up notes-related code around `load_subtree()`
> On 10 Sep 2017, at 09:39, Jeff King wrote: > > On Sun, Sep 10, 2017 at 06:45:08AM +0200, Michael Haggerty wrote: > >>> So nothing to see here, but since I spent 20 minutes scratching my head >>> (and I know others look at Coverity output and may scratch their heads >>> too), I thought it was worth writing up. And also if I'm wrong, it would >>> be good to know. ;) >> >> Thanks for looking into this. I agree with your analysis. >> >> I wonder whether it is the factor of two between path lengths and byte >> lengths that is confusing Coverity. Perhaps the patch below would help. >> It requires an extra, superfluous, check, but perhaps makes the code a >> tad more readable. I'm neutral on whether we would want to make the change. > > Yeah, I do agree that it makes the code's assumptions a bit easier to > follow. > >> Is there a way to ask Coverity whether a hypothetical change would >> remove the warning, short of merging the change to master? > > You can download and run the build portion of the coverity tools > yourself. IIRC, that pushes the build up to their servers which then do > the analysis (you can make your own "project", or use the existing "git" > project -- I checked and you are already listed as an admin). I recall > it being a minor pain to get it set up, but not too bad. > > Stefan runs it against "pu" on a regular basis, which is where the > emailed results come from. So just having Junio merge it to "pu" would > be enough to get results. > > I noticed that they now have some GitHub/Travis integration: > > https://scan.coverity.com/github > > I'm not sure if that is new, or if we just didn't notice it before. ;) > But that probably makes more sense to use than ad-hoc uploading (and > maybe it would make it easy for you to test personal branches, too). Coverity scans Git already: https://scan.coverity.com/projects/70 I requested access to this Coverity project to integrate into our TravisCI build. - Lars
Re: [PATCH v1 2/2] travis-ci: skip a branch build if equal tag is present
> On 11 Sep 2017, at 16:52, SZEDER Gábor wrote:
>
>> If we push a branch and a tag pointing to the HEAD of this branch,
>
> s/the HEAD of//, perhaps?
> There is no such thing as "HEAD" (all capital!) of a branch, is it?
Agreed, maybe:
"If we push a branch and a tag pointing to the tip of this branch..."
Would that be OK for you?
>> then Travis CI would run the build twice. This wastes resources and
>
> Nit: s/run the build/build and test the same tree/, to further stress
> that the two builds are redundant.
OK, will fix.
>> slows the testing.
>>
>> Add a function to detect this situation and skip the build the branch
>
> s/skip the build/skip building/ ?
OK, will fix.
>> if appropriate. Invoke this function on every build.
>>
>> Helped-by: Junio C Hamano
>> Signed-off-by: Lars Schneider
>> ---
>> ci/lib-travisci.sh | 23 +++
>> 1 file changed, 23 insertions(+)
>>
>> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
>> index 44d6ba2dd2..9c4ae9bdd0 100755
>> --- a/ci/lib-travisci.sh
>> +++ b/ci/lib-travisci.sh
>> @@ -1,5 +1,28 @@
>> # Library of functions shared by all CI scripts
>>
>> +skip_branch_tip_with_tag () {
>> +# Sometimes, a branch is pushed at the same time the tag that points
>> +# at the same commit as the tip of the branch is pushed, and building
>> +# both at the same time is a waste.
>> +#
>> +# Travis gives a tagname e.g. v2.14.0 in $TRAVIS_BRANCH when
>> +# the build is triggered by a push to a tag. Let's see if
>> +# $TRAVIS_BRANCH is exactly at a tag, and if so, if it is
>> +# different from $TRAVIS_BRANCH. That way, we can tell if
>> +# we are building the tip of a branch that is tagged and
>> +# we can skip the build because we won't be skipping a build
>> +# of a tag.
>> +
>> +if TAG=$(git describe --exact-match "$TRAVIS_BRANCH" 2>/dev/null) &&
>> +$TAG != $TRAVIS_BRANCH
>
> This must be
>
>[ $TAG != $TRAVIS_BRANCH ]
>
> otherwise the shell will rightfully complain:
>
> $ TRAVIS_BRANCH=v2.14.0 ./ci/lib-travisci.sh
> ./ci/lib-travisci.sh: line 17: v2.14.0: command not found
>
> Furthermore, I would prefer quotes around $TAG and $TRAVIS_BRANCH. If
> either one of those two variables were empty (or contain multiple
> words) at that point, the shell would complain. Now, I don't think
> that either can end up being empty, so quotes are not necessary, but
> having quotes around them would save future readers from spending
> brain cycles on this unnecessarily.
Agreed. I will fix both things!
Thanks for the review,
Lars
Re: What's cooking in git.git (Sep 2017, #02; Mon, 11)
Hi Junio, On Mon, 11 Sep 2017, Junio C Hamano wrote: > * js/rebase-i-final (2017-07-27) 10 commits > - rebase -i: rearrange fixup/squash lines using the rebase--helper > - t3415: test fixup with wrapped oneline > - rebase -i: skip unnecessary picks using the rebase--helper > - rebase -i: check for missing commits in the rebase--helper > - t3404: relax rebase.missingCommitsCheck tests > - rebase -i: also expand/collapse the SHA-1s via the rebase--helper > - rebase -i: do not invent onelines when expanding/collapsing SHA-1s > - rebase -i: remove useless indentation > - rebase -i: generate the script via rebase--helper > - t3415: verify that an empty instructionFormat is handled as before > > The final batch to "git rebase -i" updates to move more code from > the shell script to C. > > Expecting a reroll. No you don't: xmqqfudqkcux@gitster.mtv.corp.google.com > [...] > > -- > [Cooking] > > [...] > > * mk/diff-delta-uint-may-be-shorter-than-ulong (2017-08-10) 1 commit > . diff-delta: fix encoding size that would not fit in "unsigned int" > > The machinery to create xdelta used in pack files received the > sizes of the data in size_t, but lost the higher bits of them by > storing them in "unsigned int" during the computation, which is > fixed. > > Dropped, as it was rerolled for review as part of a larger series. > cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause> > > [...] > > * mk/use-size-t-in-zlib (2017-08-10) 1 commit > . zlib.c: use size_t for size > > The wrapper to call into zlib followed our long tradition to use > "unsigned long" for sizes of regions in memory, which have been > updated to use "size_t". > > Dropped, as it was rerolled for review as part of a larger series. > cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause> > > > * mk/diff-delta-avoid-large-offset (2017-08-11) 1 commit > . diff-delta: do not allow delta offset truncation > > The delta format used in the packfile cannot reference data at > offset larger than what can be expressed in 4-byte, but the > generator for the data failed to make sure the offset does not > overflow. This has been corrected. > > Dropped, as it was rerolled for review as part of a larger series. > cf. <1502914591-26215-1-git-send-email-martin@mail.zuhause> > > [...] > > -- > [Discarded] > > [...] This section would be more appropriate for the three "Dropped" patch series than the "Cooking" section. Ciao, Dscho
[PATCH] help: change a message to be more precise
When the user tries to use '--help' option on an aliased command
information about the alias is printed as sshown below,
$ git co --help
`git co' is aliased to `checkout'
This doesn't seem correct as the user has aliased only 'co' and not
'git co'. This might even be incorrect in cases in which the user has
used an alias like 'tgit'.
$ tgit co --help
`git co' is aliased to `checkout'
So, make the message more precise.
Signed-off-by: Kaartic Sivaraam
---
builtin/help.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/help.c b/builtin/help.c
index 334a8494a..dbe45c596 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -438,7 +438,7 @@ static const char *check_git_cmd(const char* cmd)
alias = alias_lookup(cmd);
if (alias) {
- printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
+ printf_ln(_("'%s' is aliased to '%s'"), cmd, alias);
free(alias);
exit(0);
}
--
2.14.1.1006.g90ad9a07c
[PATCH] commit-template: change a message to be more intuitive
It's not possible to 'touch' the cut-line that is shown when the
user requests a patch in his commit template.
So, make the sentence more intuitive.
Signed-off-by: Kaartic Sivaraam
---
Just a small tweak. May or may not be worth the patch.
wt-status.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/wt-status.c b/wt-status.c
index 77c27c511..1f54b1df2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -934,7 +934,7 @@ size_t wt_status_locate_end(const char *s, size_t len)
void wt_status_add_cut_line(FILE *fp)
{
- const char *explanation = _("Do not touch the line above.\nEverything
below will be removed.");
+ const char *explanation = _("Do not edit the line above.\nEverything
below will be removed.");
struct strbuf buf = STRBUF_INIT;
fprintf(fp, "%c %s", comment_line_char, cut_line);
--
2.14.1.1006.g90ad9a07c
[PATCH/RFC] branch: strictly don't allow a branch with name 'HEAD'
Allowing a branch with a name 'HEAD' could trigger ambiguity. We could
avoid this by checking for it manually. Moreover this has been done
partially in 8efb8899c ("branch: segfault fixes and validation", 2013-02-23)
There was still a way to create a branch with name 'HEAD' by using
$ git checkout -b HEAD
Avoid such loop holes by 'strictly' checking for a branch with name HEAD.
The check is referred to as strict because it's done in a place which is
supposed to be called to ensure that a new branch name is a valid one.
Signed-off-by: Kaartic Sivaraam
---
branch.c | 3 +++
builtin/branch.c | 3 ---
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/branch.c b/branch.c
index 36541d05c..ea1050649 100644
--- a/branch.c
+++ b/branch.c
@@ -184,6 +184,9 @@ int validate_new_branchname(const char *name, struct strbuf
*ref,
if (strbuf_check_branch_ref(ref, name))
die(_("'%s' is not a valid branch name."), name);
+ if (!strcmp(name, "HEAD"))
+ die(_("it does not make sense to create 'HEAD' manually"));
+
if (!ref_exists(ref->buf))
return 0;
else if (!force && !attr_only)
diff --git a/builtin/branch.c b/builtin/branch.c
index 16d391b40..c86348d4c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -762,9 +762,6 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
int branch_existed = 0, remote_tracking = 0;
struct strbuf buf = STRBUF_INIT;
- if (!strcmp(argv[0], "HEAD"))
- die(_("it does not make sense to create 'HEAD'
manually"));
-
if (!branch)
die(_("no such branch '%s'"), argv[0]);
--
2.14.1.1006.g90ad9a07c
hello..
>From The Regional Manager!, best regards!, How are you doing including your family, hope all is well? My purpose of contacting you is to crave your indulgence to assist me in securing some funds abroad. (US$7,500,000.00 ) I am writing you this proposal in good faith, believing that I can trust in you with the information I am about to reveal to you. As you indicating your interest, I will send you the full information about how the business will be executed. please contact me at this email address: ( private.jkwam...@gmail.com ) Sincerely, Mr.Johnson Kwame.
Re: [PATCH 4/4] imap-send: use curl by default
Le 12/09/2017 à 09:02, Junio C Hamano a écrit : > Junio C Hamano writes: > >> I was sweeping my mailbox to collect loose ends that haven't been >> tied down, and noticed that this topic does not seem to have reached >> a conclusion. Do we want to reboot the effort? Or should we just >> throw it in the #leftoverbits bin for now? > Ahh, I failed to find newer incarnations of this topic (there was v3 that > starts at <087f5907-6558-ce32-2f5c-2e418522c...@morey-chaisemartin.com>) > when I wrote it. Sorry about pinging a wrong/old article. > > I just gave a cursory re-read of the review thread over the v3 > series; it appears that we were very close to the finishing line > already? > > Or am I yet missing/forgetting some later developments that made > this topic obsolete? > > Thanks. > v3 needs just a few bits fixed: - Fix the bads return code in patch #1. - Reword patch #4 was you found confusing. I sent a proposal fix that you probably missed: Is something like this clearer ? Author: Nicolas Morey-Chaisemartin Date: Sun Aug 6 21:30:15 2017 +0200 imap-send: use curl by default when possible Set curl as the runtime default when it is available. When linked against older curl versions (< 7_34_0) or without curl, use the legacy imap implementation. The goal is to validate feature parity between the legacy and the curl implementation, deprecate the legacy implementation later on and in the long term, hopefully drop it altogether. Signed-off-by: Nicolas Morey-Chaisemartin If you're happy with this log message, I've v4 ready to post. Nicolas
Re: [PATCH 4/4] imap-send: use curl by default
Junio C Hamano writes: > I was sweeping my mailbox to collect loose ends that haven't been > tied down, and noticed that this topic does not seem to have reached > a conclusion. Do we want to reboot the effort? Or should we just > throw it in the #leftoverbits bin for now? Ahh, I failed to find newer incarnations of this topic (there was v3 that starts at <087f5907-6558-ce32-2f5c-2e418522c...@morey-chaisemartin.com>) when I wrote it. Sorry about pinging a wrong/old article. I just gave a cursory re-read of the review thread over the v3 series; it appears that we were very close to the finishing line already? Or am I yet missing/forgetting some later developments that made this topic obsolete? Thanks.
Re: [PATCH v3 1/2 / RFC] builtin/branch: stop supporting the use of --set-upstream option
On Tue, 2017-09-12 at 15:49 +0900, Junio C Hamano wrote: > Kaartic Sivaraam writes: > > > Thanks. Now I get it. What about doing that check in > > branch.c::create_branch or branch.c::validate_new_branchname? I guess > > creating a branch named HEAD isn't that good an idea in any case. Doing > > the check there might prevent a similar situation in future, I guess. > > Further "branch" and "checkout" do call branch.c::create_branch which > > in turn calls branch.c::validate_new_branchname. > > The above analysis sounds sensible, so it appears that you already > found a function that is shared in the two codepaths, and have a > good plan to make them consistent? > Yes, I was just waiting for this reply. In the mean time I thought of sending a patch for this but was procrastinating as I felt a little lazy. > I was sweeping my mailbox to collect loose ends that haven't been > tied down, and noticed that this topic does not seem to reach a > conclusion. Do we want to reboot the effort? Or should we just > throw it in the #leftoverbits bin for now? > Don't worry I'll send a patch for this, soon. I mean it :) -- Kaartic
