Re: git merge-tree: bug report and some feature requests

[Josh Bleecher Snyder]

>> I'm experimenting with some new porcelain for interactive rebase. One
>> goal is to leave the work tree untouched for most operations. It looks
>> to me like 'git merge-tree' may be the right plumbing command for
>> doing the merge part of the pick work of the todo list, one commit at
>> a time. If I'm wrong about this, I'd love pointers; what follows may
>> still be interesting anyway.
>
> I don't have a concrete alternative (yet?) but here are some pointers
> to two alternate merge-without-touching-working-tree possibilities, if
> your current route doesn't pan out as well as you like:
>
> I posted some patches last year to make merge-recursive.c be able to
> do merges without touching the working tree.  Adding a few flags would
> then enable it for any of 'merge', 'cherry-pick', 'am', or
> 'rebase'...though for unsuccessful merges, there's a clear question of
> what/how conflicts should be reported to the user.  That probably
> depends a fair amount on the precise use-case.
>
> Although that series was placed on the backburner due to the immediate
> driver of the feature going away, I'm still interested in such a
> change, though I think it would fall out as a nice side effect of
> implementing Junio's proposed ideal-world-merge-recursive rewrite[1].
> I have started looking into that[2], but no guarantees about how
> quickly I'll find time to finish or even whether I will.
>
> [1] https://public-inbox.org/git/xmqqd147kpdm@gitster.mtv.corp.google.com
> [2] https://github.com/newren/git/blob/ort/ort-cover-letter contains
> overview of ideas and notes to myself about what I was hoping to
> accomplish; currently it doesn't even compile or do anything

Thanks for the pointer. That does seem promising.

And yes, I see now that serialization of conflicts is decidedly
challenging. More on that below.


>> 4. API suggestion
>>
>> Here's what I really want 'git merge-tree' to output. :)
> ...
>> If the merge had conflicts, write the "as merged as possible" tree to
>
> You'd need to define "as merged as possible" more carefully, because I
> thought you meant a tree containing all the three-way merge conflict
> markers and such being present in the "resolved" file, but from your
> parenthetical note below it appears you think that is a different tree
> that would also be useful to diff against the first one.  That leaves
> me wondering what the first tree is. (Is it just the tree where for
> each path, if that path had no conflicts associated with it then it's
> the merge-resolved-file, and otherwise it's the file contents from the
> merge-base?).

FWIW, the parenthetical suggestion was indeed what I had in mind. But
non-content conflicts appear to make that a non-starter. Or at least
woefully incomplete.


> Both of these trees are actually rather non-trivial to define.  The
> wording above isn't actually sufficient, because content conflicts
> aren't the only kind of conflict.  More on that below.
>
> There is already a bunch of code in merge-recursive.c to create a
> forcibly-merged-accepting-conflict-markers-in-the-resolution and
> record it as a tree (this is used for creating virtual merge bases in
> the recursive case, namely when there isn't a single merge-base for
> the two branches you are merging).  It might be reusable for what you
> want here, but it's not immediately clear whether all the things it
> does are appropriate; someone would have to consider the non-content
> (path-based) conflicts carefully.

Ack. I assume this is also the code that generates the existing 'git
merge-tree' patches, which includes conflict markers.


>> the object database and give me its sha, and then also give me the
>> three-way merge diff output for all conflicts, as a regular patch
>> against that tree, using full path names and shas. (Alternatively,
>> maybe better, give me a second sha for a tree containing all the
>> three-way merge diff patches applied, which I can diff against the
>> first tree to find the conflict patches.)
>
> As far as I can tell, you're assuming that it's possible with two
> trees that are crafted "just right", that you can tell where the merge
> conflicts are, with binary files being your only difficulty.  Content
> conflicts aren't the only type that exist; there are also path-based
> conflicts.  These type of conflicts also make it difficult to know how
> the two trees you are requesting should even be created.
>
> For example, if there is a modify/delete conflict, how can that be
> determined from just two trees?  If the first tree has the base
> version of the file, then the second tree either has a file at the
> same position or it doesn't.  Neither case looks like a conflict, but
> the original merge had one.  You need more information.  The exact
> same thing can be said for rename/delete conflicts.
>
> Similarly, rename/add (one side renames an existing file to some new
> path (say, "new_path"), and the other adds a brand new file at
> "new_path), or rename/rename(2to1) (each 

Re: [PATCH] Fixes compile warning with -Wimplicit-fallthrough CFLAGS

[Jacob Keller]

On Mon, Jan 22, 2018 at 4:59 PM, Jeff King  wrote:
> On Mon, Jan 22, 2018 at 07:54:18PM -0500, Eric Sunshine wrote:
>
>> On Mon, Jan 22, 2018 at 6:51 PM, Elia Pinto  wrote:
>> > This patch add explicit fallthrough compiler attribute
>> > when needed on switch case statement eliminating
>> > the compile warning [-Werror=implicit-fallthrough=].
>> > It does this by means of a macro that takes into account
>> > the versions of the compilers that include that attribute.
>> > [...]
>> > Signed-off-by: Elia Pinto 
>> > ---
>> > diff --git a/convert.c b/convert.c
>> > @@ -1554,7 +1554,7 @@ static int ident_filter_fn(struct stream_filter 
>> > *filter,
>> > switch (ident->state) {
>> > default:
>> > strbuf_add(>left, head, ident->state);
>> > -   /* fallthrough */
>> > +   GIT_FALLTHROUGH;
>> > case IDENT_SKIPPING:
>> > /* fallthrough */
>>
>> Why doesn't this /* fallthrough */ deserve the same treatment?
>>
>> > case IDENT_DRAINING:
>
> I can't answer that philosophically, but I can tell you why the compiler
> does not complain. :)
>
> Usually case arms with no statements between them are exempt from
> fallthrough warnings. So:
>
>   switch (foo)
>   case 1:
>   case 2:
>   case 3:
>  /* do one thing */
>  break;
>   case 4:
>  /* do another thing */
>  break;
>   }
>
> does not need any annotations for cases 1 and 2 to fallthrough. Which
> means that the original comment was not actually necessary for the
> compiler, though the original author considered it a useful comment.
>
> So there you get into philosophy. Should it be converted to a
> compiler-visible annotation, or is it better left as a comment?
>
> -Peff

I'd personally rather stick to the comment if we can, or use something
like "fallthrough;" to make it appear like a keyword, instead of an
all caps macro, since at least to my sensibility, the all caps is a
bit too crazy.

Also, I would not put one inside an empty case statement that just
falls through to the next branch and does nothing special itself.

Thanks,
Jake

Re: [PATCH] enable core.fsyncObjectFiles by default

[Theodore Ts'o]

On Mon, Jan 22, 2018 at 07:47:10PM -0500, Jeff King wrote:
> 
> I think Ævar is talking about the case of:
> 
>   1. You make 100 objects that aren't referenced. They're loose.
> 
>   2. You run git-gc. They're still too recent to be deleted.
> 
> Right now those recent loose objects sit loose, and have zero cost at
> the time of gc.  In a "cruft pack" world, you'd pay some I/O to copy
> them into the cruft pack, and some CPU to zlib and delta-compress them.
> I think that's probably fine, though.

I wasn't assuming that git-gc would create a cruft pack --- although I
guess it could.  As you say, recent loose objects have relatively zero
cost at the time of gc.  To the extent that the gc has to read lots of
loose files, there may be more seeks in the cold cache case, so there
is actually *some* cost to having the loose objects, but it's not
great.

What I was thinking about instead is that in cases where we know we
are likely to be creating a large number of loose objects (whether
they referenced or not), in a world where we will be calling fsync(2)
after every single loose object being created, pack files start
looking *way* more efficient.  So in general, if you know you will be
creating N loose objects, where N is probably around 50 or so, you'll
want to create a pack instead.

One of those cases is "repack -A", and in that case the loose objects
are all going tobe not referenced, so it would be a "cruft pack".  But
in many other cases where we might be importing from another DCVS,
which will be another case where doing an fsync(2) after every loose
object creation (and where I have sometimes seen it create them *all*
loose, and not use a pack at all), is going to get extremely slow and
painful.

> So if we pack all the loose objects into a cruft pack, the mtime of the
> cruft pack becomes the new gauge for "recent". And if we migrate objects
> from old cruft pack to new cruft pack at each gc, then they'll keep
> getting their mtimes refreshed, and we'll never drop them.

Well, I was assuming that gc would be a special case which doesn't the
mtime of the old cruft pack.  (Or more generally, any time an object
is gets copied out of the cruft pack, either to a loose object, or to
another pack, the mtime on the source pack should not be touched.)

   - Ted

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Duy Nguyen]

On Tue, Jan 23, 2018 at 6:09 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
>> index af9b847761..d2a8e0312a 100755
>> --- a/t/t1700-split-index.sh
>> +++ b/t/t1700-split-index.sh
>> @@ -401,4 +401,23 @@ done <<\EOF
>>  0642 -rw-r---w-
>>  EOF
>>
>> +test_expect_success SANITY 'graceful handling when splitting index is not 
>> allowed' '
>
> Is SANITY the only prereq we want, or do we want both it and POSIXPERM?
>
> In "git grep SANITY t/" output, we see that they are almost always
> used together.

SANITY test does more or less the same as this one (chmod then verify)
which is the reason I removed POSIXPERM. Looking at other tests
though, they don't do anything different than what I do here and still
require both SANITY and POSIXPERM. I'm adding POSIXPERM back.

>
>> + test_create_repo ro &&
>> + (
>> + cd ro &&
>> + test_commit initial &&
>> + git update-index --split-index &&
>> + test -f .git/sharedindex.*
>> + ) &&
>> + cp ro/.git/index new-index &&
>> + test_when_finished "chmod u+w ro/.git" &&
>> + chmod u-w ro/.git &&
>> + GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index 
>> &&
>> + chmod u+w ro/.git &&
>> + rm ro/.git/sharedindex.* &&
>> + GIT_INDEX_FILE=new-index git ls-files >actual &&
>> + echo initial.t >expected &&
>> + test_cmp expected actual
>> +'
>> +
>>  test_done



-- 
Duy

Re: The original file that was split in 2 other files, is there a way in git to see what went where?

[Aleksey Bykov]

Hello,

My problem:

I am a code reviewer, I have a situation in GIT:

- before: a.txt

Then a developer decided to split the content of a.txt into 2 files
and add a few changes all in one commit:

- after: b.txt + few changes and c.txt + few changes

Is there an easy way to see:

1. what came to b from a?
2 .what came to c from a?
3. all extra changes apart from just moving stuff?

A specific command would help a lot.

A certain policy/workflow that prevents from problem like this (when
there is no way to visually diff the changes) would also help.

https://stackoverflow.com/questions/48350398/the-original-file-that-was-split-in-2-other-files-is-there-a-way-in-git-to-see


Thanks,
Aleksey Bykov

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Duy Nguyen]

On Tue, Jan 23, 2018 at 6:52 AM, Jeff King  wrote:
> On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> Patches or cover letters generated by format-patch are meant to be
>> exchanged as emails, most of the time. And since it's generally agreed
>> that text in mails should be wrapped around 70 columns or so, make sure
>> these diffstat follow the convention.
>>
>> I noticed this when I quoted a diffstat line [1]. Should we do something
>> like this? diffstat is rarely quoted though so perhaps the stat width
>> should be something like 75.
>
> I think the general idea is sensible. Somewhere I picked up "72" as the
> right size for email lines to accommodate quoting, but I'm pretty sure
> you could justify any number between 70 and 75. :)

I think it's easy to settle on 72 because cover letter's shortlog
already wraps at 72 columns. No point in introducing another number
here.

> PS I had a funny feeling that this had come up before not due to
>quoting, but just due to people with enormous terminals generating
>too-long lines. But I couldn't find any discussion, and my
>(admittedly brief) reading of the code is that we'd actually respect
>the terminal size by default.

Yeah, there are tests to check that we do ignore terminal size too.
-- 
Duy

Re: [PATCH v2 07/14] match-trees: convert splice_tree to object_id

[Duy Nguyen]

On Mon, Jan 22, 2018 at 02:12:56PM +0100, Patryk Obara wrote:
> >> @@ -197,26 +195,26 @@ static int splice_tree(const unsigned char *hash1,
> >>   if (strlen(name) == toplen &&
> >>   !memcmp(name, prefix, toplen)) {
> >>   if (!S_ISDIR(mode))
> >> - die("entry %s in tree %s is not a tree",
> >> - name, sha1_to_hex(hash1));
> >> - rewrite_here = (unsigned char *) oid->hash;
> >> + die("entry %s in tree %s is not a tree", 
> >> name,
> >> + oid_to_hex(hash1));
> >> + rewrite_here = (struct object_id *)oid;
> >
> > You don't need the typecast here anymore, do you?
> 
> Unfortunately, I do :(
> 
> Few lines above:
> 192: const struct object_id *oid;
> 194: oid = tree_entry_extract(, , );
> 
> Function tree_entry_extract returns const pointer, which leads to
> compiler warning:
> "assigning to 'struct object_id *' from 'const struct object_id *'
> discards qualifiers".
> 
> On the other hand, if I change const qualifier for 'rewrite_here'
> variable - warning will
> appear in line 216:
> 
> 216: oidcpy(rewrite_here, rewrite_with);
> 
> So the question here is rather: is it ok to overwrite buffer returned
> by tree_entry_extract?
> 
> When writing this I opted to preserve cv-qualifiers despite changing
> pointer type (which implied preservation of typecast) - partly
> because parameter 'desc' of tree_entry_extract is NOT const (which
> suggests to me, that it's ok).
> 
> But this cast might be indication of unintended modification inside
> tree description structure and might lead to an error is some other
> place, if there's an assumption, that this buffer is not
> overwritable.
> 
> Maybe const should be removed from return type of tree_entry_extract
> (and maybe from oid field of struct name_entry)?
>
> I will give it some more thought - maybe oidcpy from line 216 could
> be replaced.

I've read this code a bit more (sorry I didn't see the "const struct
object_id *oid" line when I read this patch). I think the typecast is
very much on purpose. Junio wanted to make a new tree with one
different hash in 68faf68938 (A new merge stragety 'subtree'. -
2007-02-15) but I think he kinda abused the tree walker for this
task.

A cleaner way is create a new tree by copying unmodified entries and
replacing just one entry. I think the old way was ok when we dealt
with SHA-1 directly, but with the object_id abstraction in place, this
kind of update looks iffy.

Alternatively, perhaps we can do something like this to keep tree
manipulation in tree-walk.c, one of the two places that know about
tree object on-disk format (the other one is cache-tree.c)

-- 8< --
diff --git a/match-trees.c b/match-trees.c
index 396b7338df..a8dc8a53d9 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -171,7 +171,7 @@ static int splice_tree(const unsigned char *hash1,
char *buf;
unsigned long sz;
struct tree_desc desc;
-   unsigned char *rewrite_here;
+   const object_id *rewrite_here;
const unsigned char *rewrite_with;
unsigned char subtree[20];
enum object_type type;
@@ -199,7 +199,7 @@ static int splice_tree(const unsigned char *hash1,
if (!S_ISDIR(mode))
die("entry %s in tree %s is not a tree",
name, sha1_to_hex(hash1));
-   rewrite_here = (unsigned char *) oid->hash;
+   rewrite_here = oid->hash;
break;
}
update_tree_entry();
@@ -215,7 +215,7 @@ static int splice_tree(const unsigned char *hash1,
}
else
rewrite_with = hash2;
-   hashcpy(rewrite_here, rewrite_with);
+   replace_tree_entry_hash(, rewrite_with, buf, sz);
status = write_sha1_file(buf, sz, tree_type, result);
free(buf);
return status;
diff --git a/tree-walk.c b/tree-walk.c
index 63a87ed666..f31a03569f 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -164,6 +164,17 @@ int tree_entry_gently(struct tree_desc *desc, struct 
name_entry *entry)
return 1;
 }
 
+void replace_tree_entry_hash(struct tree_desc *desc,
+const unsigned char *sha1,
+char *buf, unsigned long size)
+{
+   unsigned long offset = (const char *)desc->buffer - buf;
+   unsigned char *to_update;
+
+   to_update = (unsigned char *)buf + offset + 
tree_entry_len(>entry);
+   hashcpy(to_update, sha1);
+}
+
 void setup_traverse_info(struct traverse_info *info, const char *base)
 {
int pathlen = strlen(base);
diff --git a/tree-walk.h b/tree-walk.h
index b6bd1b4ccf..9a7d133d68 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -35,6 +35,9 @@ int update_tree_entry_gently(struct tree_desc *);
 void init_tree_desc(struct tree_desc *desc, 

Re: [PATCH] Fixes compile warning with -Wimplicit-fallthrough CFLAGS

[Jeff King]

On Mon, Jan 22, 2018 at 07:54:18PM -0500, Eric Sunshine wrote:

> On Mon, Jan 22, 2018 at 6:51 PM, Elia Pinto  wrote:
> > This patch add explicit fallthrough compiler attribute
> > when needed on switch case statement eliminating
> > the compile warning [-Werror=implicit-fallthrough=].
> > It does this by means of a macro that takes into account
> > the versions of the compilers that include that attribute.
> > [...]
> > Signed-off-by: Elia Pinto 
> > ---
> > diff --git a/convert.c b/convert.c
> > @@ -1554,7 +1554,7 @@ static int ident_filter_fn(struct stream_filter 
> > *filter,
> > switch (ident->state) {
> > default:
> > strbuf_add(>left, head, ident->state);
> > -   /* fallthrough */
> > +   GIT_FALLTHROUGH;
> > case IDENT_SKIPPING:
> > /* fallthrough */
> 
> Why doesn't this /* fallthrough */ deserve the same treatment?
> 
> > case IDENT_DRAINING:

I can't answer that philosophically, but I can tell you why the compiler
does not complain. :)

Usually case arms with no statements between them are exempt from
fallthrough warnings. So:

  switch (foo)
  case 1:
  case 2:
  case 3:
 /* do one thing */
 break;
  case 4:
 /* do another thing */
 break;
  }

does not need any annotations for cases 1 and 2 to fallthrough. Which
means that the original comment was not actually necessary for the
compiler, though the original author considered it a useful comment.

So there you get into philosophy. Should it be converted to a
compiler-visible annotation, or is it better left as a comment?

-Peff

Re: [PATCH] Fixes compile warning with -Wimplicit-fallthrough CFLAGS

[Eric Sunshine]

On Mon, Jan 22, 2018 at 6:51 PM, Elia Pinto  wrote:
> This patch add explicit fallthrough compiler attribute
> when needed on switch case statement eliminating
> the compile warning [-Werror=implicit-fallthrough=].
> It does this by means of a macro that takes into account
> the versions of the compilers that include that attribute.
> [...]
> Signed-off-by: Elia Pinto 
> ---
> diff --git a/convert.c b/convert.c
> @@ -1554,7 +1554,7 @@ static int ident_filter_fn(struct stream_filter *filter,
> switch (ident->state) {
> default:
> strbuf_add(>left, head, ident->state);
> -   /* fallthrough */
> +   GIT_FALLTHROUGH;
> case IDENT_SKIPPING:
> /* fallthrough */

Why doesn't this /* fallthrough */ deserve the same treatment?

> case IDENT_DRAINING:

Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute

[Jeff King]

On Mon, Jan 22, 2018 at 01:35:25PM +0100, Lars Schneider wrote:

> >> +  enc = xcalloc(1, sizeof(struct convert_driver));
> > 
> > I think this should be "sizeof(struct encoding)" but I prefer
> > "sizeof(*enc)" which prevents these kind of mistakes.
> 
> Great catch! Thank you!
> 
> Other code paths are at risk of this problem too. Consider this:
> 
> $ git grep 'sizeof(\*' | wc -l
>  303
> $ git grep 'sizeof(struct ' | wc -l
>  208
> 
> E.g. even in the same file (likely where I got the code from):
> https://github.com/git/git/blob/59c276cf4da0705064c32c9dba54baefa282ea55/convert.c#L780
> 
> @Junio: 
> Would you welcome a patch that replaces "struct foo" with "*foo"
> if applicable?

This is part of the reason we've been moving to helpers like
ALLOC_ARRAY(), which make it harder to get this wrong.

We don't have an ALLOC_OBJECT(), which is what you would want here. I'm
not sure if that is helpful or crossing the line of "you're obscuring it
to the point that people familiar with C have trouble reading the code".
The ALLOC_ARRAY() macros have been sort of an experiment there (I tend
to like them, but I also work with Git's code often enough that I am not
likely to be confused by our bespoke macros).

But anyway, that was a bit of a tangent. Certainly the smaller change is
just standardizing on sizeof(*foo), which I think most people agree on
at this point. It might be worth putting in CodingGuidelines.

-Peff

Re: [PATCH] enable core.fsyncObjectFiles by default

[Jeff King]

On Mon, Jan 22, 2018 at 01:09:03PM -0500, Theodore Ts'o wrote:

> > Wouldn't it also make gc pruning more expensive? Now you can repack
> > regularly and loose objects will be left out of the pack, and then just
> > rm'd, whereas now it would entail creating new packs (unless the whole
> > pack was objects meant for removal).
> 
> The idea is that the cruft pack would be all objects that were no
> longer referenced.  Hence the proposal that if they ever *are*
> accessed, they would be exploded to a loose object at that point.  So
> in the common case, the GC would go quickly since the entire pack
> could just be rm'ed once it hit the designated expiry time.

I think Ævar is talking about the case of:

  1. You make 100 objects that aren't referenced. They're loose.

  2. You run git-gc. They're still too recent to be deleted.

Right now those recent loose objects sit loose, and have zero cost at
the time of gc.  In a "cruft pack" world, you'd pay some I/O to copy
them into the cruft pack, and some CPU to zlib and delta-compress them.
I think that's probably fine, though.

That said, some of what you wrote left me confused, and whether we're
all talking about the same idea. ;) Let me describe the idea I had
mentioned in another thread.  Right now the behavior is basically this:

If an unreachable object becomes referenced, it doesn't immediately get
exploded. During the next gc, whatever new object referenced them would
be one of:

  1. Reachable from refs, in which case it carries along the
 formerly-cruft object into the new pack, since it is now also
 reachable.

  2. Unreachable but still recent by mtime; we keep such objects, and
 anything they reference (now as unreachable, in this proposal in
 the cruft pack). Now these get either left loose, or exploded loose
 if they were previously packed.

  3. Unreachable and old. Both objects can be dropped totally.

The current strategy is to use the mtimes for "recent", and we use the
pack's mtime for every object in the pack.

So if we pack all the loose objects into a cruft pack, the mtime of the
cruft pack becomes the new gauge for "recent". And if we migrate objects
from old cruft pack to new cruft pack at each gc, then they'll keep
getting their mtimes refreshed, and we'll never drop them.

So we need to either:

  - keep per-object mtimes, so that old ones can age out (i.e., they'd
hit case 3 and just not get migrated to either the new "real" pack
or the new cruft pack).

  - keep multiple cruft packs, and let whole packs age out. But then
cruft objects which get referenced again by other cruft have to get
copied (not moved!) to new packs. That _probably_ doesn't happen all
that often, so it might be OK.

> Another way of doing things would be to use the mtime of the cruft
> pack for the expiry time, and if the curft pack is ever referenced,
> its mtime would get updated.  Yet a third way would be to simply clear
> the "cruft" bit if it ever *is* referenced.  In the common case, it
> would never be referenced, so it could just get deleted, but in the
> case where the user has manually "rescued" a set of commits (perhaps
> by explicitly setting a branch head to commit id found from a reflog),
> the objects would be saved.

I don't think we have to worry about "rescued" objects. Those are
reachable, so they'd get copied into the new "real" pack (and then their
cruft pack eventually deleted).

-Peff

Re: [PATCH] enable core.fsyncObjectFiles by default

[Jeff King]

On Mon, Jan 22, 2018 at 04:09:23PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > Yes, a "cruft pack" that holds unreachable object has been discussed
> > a few times recently on list, and I do agree that it is a desirable
> > thing to have in the longer run.
> >
> > What's tricky is to devise a way to allow us to salvage objects that
> > are placed in a cruft pack because they are accessed recently,
> > proving themselves to be no longer crufts.  It could be that a good
> > way to resurrect them is to explode them to loose form when they are
> > accessed out of a cruft pack.  We need to worry about interactions
> > with read-only users if we go that route, but with the current
> > "explode unreachable to loose, touch their mtime when they are
> > accessed" scheme ends up ignoring accesses from read-only users that
> > cannot update mtime, so it might not be too bad.
> 
> Wouldn't it also make gc pruning more expensive? Now you can repack
> regularly and loose objects will be left out of the pack, and then just
> rm'd, whereas now it would entail creating new packs (unless the whole
> pack was objects meant for removal).
> 
> Probably still worth it, but something to keep in mind.

That's a good point. I think it would be OK in practice, though, since
normal operations don't tend to create a huge number of unreachable
loose objects (at least compared to the _reachable_ loose objects, which
we're already dealing with). We tend to get unbalanced explosions of
loose objects only because a huge chunk of packed history expired.

It is something to keep in mind when implementing the scheme, though.
Luckily we already have the right behavior implemented via
--pack-loose-unreachable (which is used for "repack -k" currently), so I
think it would just be a matter of passing the right flags from
git-repack.

-Peff

[RFC PATCH 1/1] Implement CMake build

[Isaac Hier]

Signed-off-by: Isaac Hier 
---
 CMakeLists.txt  | 1849 +++
 cmake/GenerateCmdlist.cmake |   83 ++
 cmake/fopen_dir_test.c  |   11 +
 cmake/fstat_test.c  |   37 +
 cmake/gmtime_test.c |7 +
 cmake/iconv_test.c  |   13 +
 cmake/inline_test.c |9 +
 cmake/mkdir_test.c  |7 +
 cmake/mmap_test.c   |   35 +
 cmake/parens_test.c |6 +
 cmake/snprintf_test.c   |7 +
 cmake/sysctl_test.c |   11 +
 12 files changed, 2075 insertions(+)
 create mode 100644 CMakeLists.txt
 create mode 100644 cmake/GenerateCmdlist.cmake
 create mode 100644 cmake/fopen_dir_test.c
 create mode 100644 cmake/fstat_test.c
 create mode 100644 cmake/gmtime_test.c
 create mode 100644 cmake/iconv_test.c
 create mode 100644 cmake/inline_test.c
 create mode 100644 cmake/mkdir_test.c
 create mode 100644 cmake/mmap_test.c
 create mode 100644 cmake/parens_test.c
 create mode 100644 cmake/snprintf_test.c
 create mode 100644 cmake/sysctl_test.c

diff --git a/CMakeLists.txt b/CMakeLists.txt
new file mode 100644
index 0..dff3a44c8
--- /dev/null
+++ b/CMakeLists.txt
@@ -0,0 +1,1849 @@
+cmake_minimum_required(VERSION 3.3)
+
+set(CMAKE_C_STANDARD 99)
+set(CMAKE_C_STANDARD_REQUIRED ON)
+
+set(bin_dir "bin")
+set(lib_dir "lib")
+set(git_exec_dir "libexec/git-core")
+set(mergetools_dir "${git_exec_dir}/mergetools")
+set(share_dir "share")
+set(man_dir "${share_dir}/man")
+set(info_dir "${share_dir}/info")
+set(gitweb_dir "${share_dir}/gitweb")
+set(locale_dir "${share_dir}/locale")
+set(template_dir "${share_dir}/git-core/templates")
+set(html_dir "${share_dir}/doc/git-doc")
+set(etc_gitconfig "etc/gitconfig")
+set(etc_gitattributes "etc/gitattributes")
+
+find_package(Git)
+if(IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/.git" AND GIT_FOUND)
+  execute_process(COMMAND ${GIT_EXECUTABLE} rev-parse -q --verify HEAD
+  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+  OUTPUT_VARIABLE GIT_BUILT_FROM_COMMIT
+  OUTPUT_STRIP_TRAILING_WHITESPACE)
+  message(STATUS "GIT_BUILT_FROM_COMMIT: ${GIT_BUILT_FROM_COMMIT}")
+endif()
+
+if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/version" AND
+   NOT IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/version")
+  file(READ "${CMAKE_CURRENT_SOURCE_DIR}/version" GIT_VERSION)
+elseif(IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/.git" AND GIT_FOUND)
+  execute_process(COMMAND ${GIT_EXECUTABLE} describe --match "v[0-9]*" HEAD
+  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+  OUTPUT_VARIABLE GIT_VERSION
+  OUTPUT_STRIP_TRAILING_WHITESPACE)
+  if(GIT_VERSION MATCHES "v[0-9]*")
+string(REPLACE "-" "." GIT_VERSION "${GIT_VERSION}")
+execute_process(COMMAND ${GIT_EXECUTABLE} diff-index --name-only HEAD --
+WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+OUTPUT_VARIABLE git_diff_index_output
+OUTPUT_STRIP_TRAILING_WHITESPACE)
+if(git_diff_index_output)
+  set(GIT_VERSION "${GIT_VERSION}.dirty")
+endif()
+message(STATUS "GIT_VERSION: ${GIT_VERSION}")
+  else()
+message(FATAL_ERROR "Cannot determine project version")
+  endif()
+else()
+  set(GIT_VERSION "${DEFAULT_VERSION}")
+endif()
+
+string(REGEX REPLACE "^v([0-9]+\\.[0-9]+\\.[0-9]+).*$" "\\1"
+   simplified_version "${GIT_VERSION}")
+project(git VERSION "${simplified_version}" LANGUAGES C)
+message(STATUS "PROJECT_VERSION: ${PROJECT_VERSION}")
+
+set(GIT_USER_AGENT "git/${PROJECT_VERSION}" CACHE STRING
+"User-agent used for network interactions")
+message(STATUS "GIT_USER_AGENT: ${GIT_USER_AGENT}")
+
+list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
+
+include(CheckCCompilerFlag)
+include(CheckIncludeFile)
+include(CheckFunctionExists)
+include(CheckLibraryExists)
+include(CheckPrototypeDefinition)
+include(CheckStructHasMember)
+include(CheckSymbolExists)
+include(CheckTypeSize)
+include(CMakeDependentOption)
+include(CTest)
+
+include(GenerateCmdlist)
+
+generate_cmdlist()
+
+macro(print_bool arg_name)
+  if(${arg_name})
+message(STATUS "${arg_name}: true")
+  else()
+message(STATUS "${arg_name}: false")
+  endif()
+endmacro()
+
+macro(add_c_flag c_flags flag)
+  string(MAKE_C_IDENTIFIER "${flag}" flag_var)
+  check_c_compiler_flag("${flag}" have_${flag_var})
+  if(have_${flag_var})
+set(${c_flags} "${${c_flags}} ${flag}")
+  endif()
+endmacro()
+
+# Sources defined here to append to during configuration stage.
+
+set(program_src
+"credential-store.c"
+"daemon.c"
+"fast-import.c"
+"http-backend.c"
+"sh-i18n--envsubst.c"
+"shell.c"
+"show-index.c"
+"upload-pack.c")
+
+if(ENABLE_TESTING)
+  set(test_program_src
+"t/helper/test-chmtime.c"
+"t/helper/test-ctype.c"
+"t/helper/test-config.c"
+"t/helper/test-date.c"
+"t/helper/test-delta.c"
+"t/helper/test-drop-caches.c"
+ 

[RFC PATCH 0/1] Implement CMake build

[Isaac Hier]

This patch adds a mostly complete (aside from building tests, documentation,
installation, etc.) CMake build to the git project. I am not sure how much
interest there is in a CMake build, so please send me feedback one way or
another. Personally, I believe CMake will help with Windows builds and is
somewhat easier to read than a Makefile. I considered, adding this to the
contrib directory, but CMakeLists.txt almost always reside in the original
directories, and I'm not sure how wise it would be to do otherwise. If you are
interested in a CMake build, I would be more than happy to finish up the work
here. Decided to wait until I discussed the issue here to finish the final parts
of the build.

Isaac Hier (1):
  Implement CMake build

 CMakeLists.txt  | 1849 +++
 cmake/GenerateCmdlist.cmake |   83 ++
 cmake/fopen_dir_test.c  |   11 +
 cmake/fstat_test.c  |   37 +
 cmake/gmtime_test.c |7 +
 cmake/iconv_test.c  |   13 +
 cmake/inline_test.c |9 +
 cmake/mkdir_test.c  |7 +
 cmake/mmap_test.c   |   35 +
 cmake/parens_test.c |6 +
 cmake/snprintf_test.c   |7 +
 cmake/sysctl_test.c |   11 +
 12 files changed, 2075 insertions(+)
 create mode 100644 CMakeLists.txt
 create mode 100644 cmake/GenerateCmdlist.cmake
 create mode 100644 cmake/fopen_dir_test.c
 create mode 100644 cmake/fstat_test.c
 create mode 100644 cmake/gmtime_test.c
 create mode 100644 cmake/iconv_test.c
 create mode 100644 cmake/inline_test.c
 create mode 100644 cmake/mkdir_test.c
 create mode 100644 cmake/mmap_test.c
 create mode 100644 cmake/parens_test.c
 create mode 100644 cmake/snprintf_test.c
 create mode 100644 cmake/sysctl_test.c

-- 
2.14.1

Re: [PATCH v5] mru: Replace mru.[ch] with list.h implementation

[Jeff King]

On Mon, Jan 22, 2018 at 07:37:01AM +, Eric Wong wrote:

> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -859,9 +859,8 @@ static void prepare_packed_git_mru(void)
> >  {
> > struct packed_git *p;
> >  
> > -   mru_clear(_git_mru);
> 
> But the removed mru_clear needs to be replaced with:
> 
> + INIT_LIST_HEAD(_git_mru);
> 
> Otherwise, t3050 never finishes for me.

Good catch. One alternative is to just add any new packs to the end (or
beginning) of the mru list, instead of "resetting" it here. We can do
that because we know that prepare_packed_git() never drops list entries,
but only adds new ones.

I'm OK with it either way, though.

-Peff

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Jeff King]

On Tue, Jan 23, 2018 at 01:10:43AM +0100, Ævar Arnfjörð Bjarmason wrote:

> 
> On Mon, Jan 22 2018, Jeff King jotted:
> 
> > On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:
> >> +  opts.diffopt.stat_width = 70;
> >>
> >>diff_setup_done();
> >
> > I wondered how this should interact with any config, but I don't think
> > you can actually configure the stat-width. You _can_ configure
> > diff.statgraphwidth, though, which seems like a funny inconsistency.
> 
> Isn't the numeric argument to --stat (this works with/without this
> patch):
> 
> $ git format-patch -10 --stdout --stat=30 -- t|grep -m 5 ' | '
>  ...submodule-update.sh | 1 +
>  ...ule-update.sh | 14 ++
>  ...-addresses.sh | 27 ---
>  t/t9000/test.pl  | 67 --
>  ...send-email.sh | 19 ++
> $ git format-patch -10 --stdout --stat=90 -- t|grep -m 5 ' | '
>  t/lib-submodule-update.sh | 1 +
>  t/lib-submodule-update.sh | 14 ++
>  t/t9000-addresses.sh | 27 -
>  t/t9000/test.pl  | 67 
> --
>  t/t9001-send-email.sh | 19 +++

Yeah, I meant by actual on-disk config. I didn't actually look at the
patch closely, but I assumed that "format-patch --stat=90" would still
override this (if not, then I think that would be a bug).

-Peff

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 22 2018, Jeff King jotted:

> On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> +opts.diffopt.stat_width = 70;
>>
>>  diff_setup_done();
>
> I wondered how this should interact with any config, but I don't think
> you can actually configure the stat-width. You _can_ configure
> diff.statgraphwidth, though, which seems like a funny inconsistency.

Isn't the numeric argument to --stat (this works with/without this
patch):

$ git format-patch -10 --stdout --stat=30 -- t|grep -m 5 ' | '
 ...submodule-update.sh | 1 +
 ...ule-update.sh | 14 ++
 ...-addresses.sh | 27 ---
 t/t9000/test.pl  | 67 --
 ...send-email.sh | 19 ++
$ git format-patch -10 --stdout --stat=90 -- t|grep -m 5 ' | '
 t/lib-submodule-update.sh | 1 +
 t/lib-submodule-update.sh | 14 ++
 t/t9000-addresses.sh | 27 -
 t/t9000/test.pl  | 67 
--
 t/t9001-send-email.sh | 19 +++

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 22 2018, Nguyễn Thái Ngọc Duy jotted:

> diff --git a/builtin/log.c b/builtin/log.c
> index 14fdf39165..6be79656c5 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int 
> use_stdout,
>  
>   memcpy(, >diffopt, sizeof(opts));
>   opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> + opts.diffopt.stat_width = 70;
>  
>   diff_setup_done();

There's no opts.diffopt. Presumably this should be squashed on top:

-   opts.diffopt.stat_width = 70;
+   rev->diffopt.stat_width = 70;

Re: [PATCH] Fixes compile warning with -Wimplicit-fallthrough CFLAGS

[Jeff King]

On Mon, Jan 22, 2018 at 11:51:18PM +, Elia Pinto wrote:

> This patch add explicit fallthrough compiler attribute
> when needed on switch case statement eliminating
> the compile warning [-Werror=implicit-fallthrough=].
> It does this by means of a macro that takes into account
> the versions of the compilers that include that attribute.
> 
> The fallthrough (or clang::fallthrough) attribute is used to annotate
> intentional fall-through between switch labels.
> Traditionally these are marked with a specific comment, but
> this attribute is meant to replace comments with a more strict
> annotation, which can be checked by the compiler (gcc-7 or clang).
> The flags in question were introduced in gcc 7 and are also enabled
> with -Wextra.

Hrm. Your subject says "fixes compile warnings", but don't we already
compile cleanly with -Wimplicit-fallthrough after my 1cf01a34ea
(consistently use "fallthrough" comments in switches, 2017-09-21)?

Certainly the tip of "master" seems to pass for me on both gcc 7 and
clang 4. You can pump the warning up to level 5 on gcc to insist on the
attribute, but I think the comments are more readable (and it is not
like we have a problem with false positive comments).

> It would also have been possible to introduce a specific comment
> accepted by gcc 7 instead of the fallthrough attribute for this warning,
> but clang does not have a similar implementation. The macro replaces
> the previous, not uniform, comments and can acts as a comment itself.

Interestingly clang seems to accept -Wimplicit-fallthrough, but I could
not get it to actually trigger a warning, even after removing some of
the existing comments.

What version of clang are you using? I'm certainly puzzled by the
behavior I'm seeing.

> diff --git a/apply.c b/apply.c
> index 321a9fa68..a22fb2881 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1450,7 +1450,7 @@ static void recount_diff(const char *line, int size, 
> struct fragment *fragment)
>   switch (*line) {
>   case ' ': case '\n':
>   newlines++;
> - /* fall through */
> + GIT_FALLTHROUGH;

Ugh, the semi-colon there makes it look like it's actual code. If we go
this route, I wonder if it's worth hiding it inside the macro.

-Peff

RE: [Nit] Lots of enumerated type warnings

[Randall S. Becker]

On January 22, 2018 6:44 PM, Junio C Hamano wrote:
> 
> "Randall S. Becker"  writes:
> 
> > Here are a few examples, there are more:
> >
> > auto_crlf = git_config_bool(var, value);
> >   ^
> 
> The carets in your message do not align to what I think they are trying to
> point at, but I think the above is pointing at the '=' and wants to say
> "auto_crlf variable is enum, it gets assigned an integer returned from
> git_config_bool(), and I do not like that assignment".
> 
> For this one I tend to agree with the compiler, meaning that it is ugly to
> define "enum auto_crlf" in such a way that the first two values happen to
> match what a logically different "enum" (which is
> "boolean") assigns the two values to.  And this judgment does not change
> whether git_config_bool() actually returns an enum or an int (the code in
> reality returns the latter).
> 
> I do not think people would terribly mind a patch to turn the above
> into:
> 
>   auto_crlf = git_config_bool(var, value) ? AUTO_CRLF_FALSE :
> AUTO_CRLF_TRUE;
> 
> > "/home/jenkins/.jenkins/workspace/Build_Git/config.c", line 1147:
> > warning(272):
> >   enumerated type mixed with another type
> >
> > type = sha1_object_info(s->oid.hash, >size);
> >  ^
> 
> /* returns enum object_type or negative */ int sha1_object_info(const
> unsigned char *sha1, unsigned long *sizep)
> 
> The function has been like this forever, I suspect, and I would say "this
gives
> negative when error, or enum we know is non-negative" is quite a
> reasonable thing to do, but the enum has OBJ_BAD defined to be negative,
> so probably it is more kosher if sha1_object_info() is declared to return
> "enum object_type" instead of int.
> 
> > "/home/jenkins/.jenkins/workspace/Build_Git/diff.c", line 3618:
> > warning(272):
> >   enumerated type mixed with another type
> >
> > options->color_moved = diff_color_moved_default;
> >  ^
> 
> If color_moved field is declared to be an enum, the _default variable
should
> be, too.  I do not think it is such a controversial fix.

The basic idea of the request is whether to slowly take on this type of
change. It will likely take a bit of time, but I really don't like warnings,
so am willing to work on it. There are loads more like this that might need
discussion, so I'll be pretty conservative on this effort.

Cheers,
Randall

Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 22 2018, Lucas Werkmeister jotted:

> Several options imply --syslog, without there being a way to disable it
> again. This commit adds that option.

Just two options imply --syslog, --detach & --inetd, unless I've missed
something, anyway 2 != several, so maybe just say "The --detach and
--inetd options imply --syslog ...".

> This is useful, for instance, when running `git daemon` as a systemd
> service with --inetd. systemd connects stderr to the journal by default,
> so logging to stderr is useful. On the other hand, log messages sent via
> syslog also reach the journal eventually, but run the risk of being
> processed at a time when the `git daemon` process has already exited
> (especially if the process was very short-lived, e.g. due to client
> error), so that the journal can no longer read its cgroup and attach the
> message to the correct systemd unit. See systemd/systemd#2913 [1].
>
> [1]: https://github.com/systemd/systemd/issues/2913

This patch looks good, but I wonder if with the rise of systemd there's
a good reason to flip the default around to not having other stuff imply
--syslog, and have users specify this implictly, then we won't need a
--no-syslog option.

But maybe that'll break too much stuff.

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Jeff King]

On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Patches or cover letters generated by format-patch are meant to be
> exchanged as emails, most of the time. And since it's generally agreed
> that text in mails should be wrapped around 70 columns or so, make sure
> these diffstat follow the convention.
> 
> I noticed this when I quoted a diffstat line [1]. Should we do something
> like this? diffstat is rarely quoted though so perhaps the stat width
> should be something like 75.

I think the general idea is sensible. Somewhere I picked up "72" as the
right size for email lines to accommodate quoting, but I'm pretty sure
you could justify any number between 70 and 75. :)

A few thoughts looking at the patch:

> diff --git a/builtin/log.c b/builtin/log.c
> index 14fdf39165..6be79656c5 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int 
> use_stdout,
>  
>   memcpy(, >diffopt, sizeof(opts));
>   opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> + opts.diffopt.stat_width = 70;
>  
>   diff_setup_done();

I wondered how this should interact with any config, but I don't think
you can actually configure the stat-width. You _can_ configure
diff.statgraphwidth, though, which seems like a funny inconsistency.

Anyway, I'm not it would make sense to prefer any kind of generic
diff.statwidth to this value. The point is that the context here has to
do with emails, not just terminals, and the rules are different. So I
think you'd need format.statwidth or something. I'm perfectly willing to
punt on that until somebody actually cares.

> @@ -1611,9 +1612,12 @@ int cmd_format_patch(int argc, const char **argv, 
> const char *prefix)
>   die(_("--check does not make sense"));
>  
>   if (!use_patch_format &&
> - (!rev.diffopt.output_format ||
> -  rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> + (!rev.diffopt.output_format ||
> +  rev.diffopt.output_format == DIFF_FORMAT_PATCH)) {
>   rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | 
> DIFF_FORMAT_SUMMARY;
> + if (!rev.diffopt.stat_width)
> + rev.diffopt.stat_width = 70;
> + }

Hmm, so if I say:

  git format-patch --stat --patch

I'd get the larger default? That seems kind of funny. Should this
stat_width setting be outside of this conditional (and if the user
asks for a non-stat format, it would just be ignored)?

-Peff

PS I had a funny feeling that this had come up before not due to
   quoting, but just due to people with enormous terminals generating
   too-long lines. But I couldn't find any discussion, and my
   (admittedly brief) reading of the code is that we'd actually respect
   the terminal size by default.

   While digging, I did find this discussion, though:

 
https://public-inbox.org/git/20080403102214.ga23...@coredump.intra.peff.net/

[PATCH] Fixes compile warning with -Wimplicit-fallthrough CFLAGS

[Elia Pinto]

This patch add explicit fallthrough compiler attribute
when needed on switch case statement eliminating
the compile warning [-Werror=implicit-fallthrough=].
It does this by means of a macro that takes into account
the versions of the compilers that include that attribute.

The fallthrough (or clang::fallthrough) attribute is used to annotate
intentional fall-through between switch labels.
Traditionally these are marked with a specific comment, but
this attribute is meant to replace comments with a more strict
annotation, which can be checked by the compiler (gcc-7 or clang).
The flags in question were introduced in gcc 7 and are also enabled
with -Wextra.

It would also have been possible to introduce a specific comment
accepted by gcc 7 instead of the fallthrough attribute for this warning,
but clang does not have a similar implementation. The macro replaces
the previous, not uniform, comments and can acts as a comment itself.

Signed-off-by: Elia Pinto 
---
 apply.c |  6 +++---
 builtin/blame.c |  2 +-
 builtin/cat-file.c  |  2 +-
 builtin/checkout.c  |  2 +-
 builtin/fast-export.c   |  4 ++--
 builtin/fetch.c |  2 +-
 builtin/init-db.c   |  2 +-
 builtin/log.c   |  4 ++--
 builtin/ls-files.c  |  2 +-
 builtin/remote-ext.c|  2 +-
 builtin/reset.c |  2 +-
 builtin/submodule--helper.c |  2 +-
 config.c|  4 ++--
 connect.c   |  6 +++---
 convert.c   |  4 ++--
 diff.c  |  2 +-
 dir.c   |  2 +-
 fsck.c  |  2 +-
 git-compat-util.h   | 17 +
 ll-merge.c  |  2 +-
 mailinfo.c  |  2 +-
 notes.c |  4 ++--
 pretty.c|  2 +-
 quote.c |  4 ++--
 read-cache.c|  4 ++--
 refs/files-backend.c|  2 +-
 revision.c  |  2 +-
 send-pack.c |  2 +-
 strbuf.c|  2 +-
 streaming.c |  2 +-
 upload-pack.c   |  3 +--
 wildmatch.c |  2 +-
 wt-status.c |  4 ++--
 33 files changed, 61 insertions(+), 45 deletions(-)

diff --git a/apply.c b/apply.c
index 321a9fa68..a22fb2881 100644
--- a/apply.c
+++ b/apply.c
@@ -1450,7 +1450,7 @@ static void recount_diff(const char *line, int size, 
struct fragment *fragment)
switch (*line) {
case ' ': case '\n':
newlines++;
-   /* fall through */
+   GIT_FALLTHROUGH;
case '-':
oldlines++;
continue;
@@ -2896,7 +2896,7 @@ static int apply_one_fragment(struct apply_state *state,
if (plen && (ws_rule & WS_BLANK_AT_EOF) &&
ws_blank_line(patch + 1, plen, ws_rule))
is_blank_context = 1;
-   /* fallthrough */
+   GIT_FALLTHROUGH;
case '-':
memcpy(old, patch + 1, plen);
add_line_info(, old, plen,
@@ -2904,7 +2904,7 @@ static int apply_one_fragment(struct apply_state *state,
old += plen;
if (first == '-')
break;
-   /* fallthrough */
+   GIT_FALLTHROUGH;
case '+':
/* --no-add does not add new lines */
if (first == '+' && state->no_add)
diff --git a/builtin/blame.c b/builtin/blame.c
index 005f55aaa..acc2c1e93 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -834,7 +834,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
argv[1] = argv[3];
argv[3] = argv[2];
argv[2] = "--";
-   /* FALLTHROUGH */
+   GIT_FALLTHROUGH;
case 1: /* (1a) */
path = add_prefix(prefix, argv[--argc]);
argv[argc] = NULL;
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index cf9ea5c79..eb4fa9247 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -113,7 +113,7 @@ static int cat_one_file(int opt, const char *exp_type, 
const char *obj_name,
 
if (textconv_object(path, obj_context.mode, , 1, , 
))
break;
-   /* else fallthrough */
+   GIT_FALLTHROUGH;
 
case 'p':
type = sha1_object_info(oid.hash, NULL);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index c54c78df5..2db0e16d1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -441,7 +441,7 @@ static int reset_tree(struct tree *tree, const struct 
checkout_opts *o,
  

Re: [PATCH v2 12/12] fetch: add a --fetch-prune option and fetch.pruneTags config

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 22 2018, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> Add a --fetch-prune option to git-fetch, along with fetch.pruneTags
>> config option. This allows for doing any of:
>>
>> git fetch -p -P
>> git fetch --prune --prune-tags
>> git fetch -p -P origin
>> git fetch --prune --prune-tags origin
>>
>> Or simply:
>>
>> git config fetch.prune true &&
>> git config fetch.pruneTags true &&
>> git fetch
>>
>> Instead of the much more verbose:
>>
>> git fetch --prune origin 'refs/tags/*:refs/tags/*' 
>> '+refs/heads/*:refs/remotes/origin/*'
>>
>> Before this feature it was painful to support the use-case of pulling
>> from a repo which is having both its branches *and* tags deleted
>> regularly, and wanting our local references to reflect upstream.
>>
>> At work we create deployment tags in the repo for each rollout, and
>> there's *lots* of those, so they're archived within weeks for
>> performance reasons.
>>
>> Without this change it's hard to centrally configure such repos in
>> /etc/gitconfig (on servers that are only used for working with
>> them). You need to set fetch.prune=true globally, and then for each
>> repo:
>>
>> git -C {} config --replace-all remote.origin.fetch 
>> "refs/tags/*:refs/tags/*" "^refs/tags/*:refs/tags/*$"
>
> I think the last one is supposed to be a regular expression on
> existing values.  Shouldn't the asterisks be quoted?
>
> Otherwise, it would appears as if "refs/tags:refs/tags///" are
> replaced with "refs/tags/*:refs/tags/*", but it certainly is not
> what you are doing.

Yes, well spotted. I copied this from an escaped version and forgot to
update that escaping. Will fix.

> I also wonder why the existing one does not expect a leading '+',
> which I think is what we place by default when you clone.

I didn't raise this because I didn't want to get side-tracked with yet
another thing, but it appears to me that a + prefix in tags refspecs
does absolutely nothing. Consider on a fresh git.git clobbering the new
v2.16.1 tag locally:

$ git tag -a -m"fake" -f v2.16.1 v2.16.0
Updated tag 'v2.16.1' (was eb5fcb24f)

This should clobber it, and does:

$ git fetch --prune --dry-run origin '+refs/tags/*:refs/tags/*'
From github.com:git/git
 t [tag update]  v2.16.1-> v2.16.1

But so will this without +, which seems like a bug to me:

$ git fetch --prune --dry-run origin 'refs/tags/*:refs/tags/*'
From github.com:git/git
 t [tag update]  v2.16.1-> v2.16.1

But maybe I'm missing something.


>> +-P::
>> +--prune-tags::
>> + This option is
>> +merely a shorthand for providing the explicit tag refspec
>> +along with `--prune`, see the discussion about that in its
>> +documentation.
>
> So would "git fetch -P origin" be like "git fetch --prune --tags
> origin"?

No, as I understand it --tags just has the effect of considering remote
tags that aren't tags of the branches you're fetching, so e.g. on a
fresh git.git:

$ git tag -a -m"fake" -f v2.16.2 v2.16.0
$ git fetch --prune --tags --dry-run
$

I.e. it does nothing. Whereas this will prune the new fake tag:

$ git fetch --prune origin 'refs/tags/*:refs/tags/*'
From github.com:git/git
 - [deleted] (none) -> v2.16.2

And so does the --prune-tags option:

$ ~/g/git/git-fetch --prune --prune-tags origin
From github.com:git/git
 - [deleted] (none) -> v2.16.2


>>  +
>>  See the PRUNING section below for more details.
>>
>> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
>> index 18fac0da2e..5682ed4ae1 100644
>> --- a/Documentation/git-fetch.txt
>> +++ b/Documentation/git-fetch.txt
>> @@ -148,6 +148,30 @@ So be careful when using this with a refspec like
>>  `refs/tags/*:refs/tags/*`, or any other refspec which might map
>>  references from multiple remotes to the same local namespace.
>>
>> +Since keeping up-to-date with both branches and tags on the remote is
>> +a common use-case the `--prune-tags` option can be supplied along with
>> +`--prune` to prune local tags that don't exist on the remote. Tag
>> +pruning can also be enabled with `fetch.pruneTags` or
>> +`remote..pruneTags` in the config. See linkgit:git-config[1].
>> +
>> +The `--prune-tags` option is equivalent to having
>> +`refs/tags/*:refs/tags/*` configured in the refspecs for the
>> +remote. This can lead to some seemingly strange interactions:
>> +
>> +
>> +# These both fetch tags
>> +$ git fetch --no-tags origin 'refs/tags/*:refs/tags/*'
>> +$ git fetch --no-tags --prune-tags origin
>> +
>
> This description is too confusing.  First you say "having
> ... configured in the refspecs for the remote", but configured
> refspecs for the remote (I presume that you are missing 'fetch' from
> that description and are talking about the 

Re: [Nit] Lots of enumerated type warnings

[Junio C Hamano]

"Randall S. Becker"  writes:

> Here are a few examples, there are more:
>
> auto_crlf = git_config_bool(var, value);
> ^

The carets in your message do not align to what I think they are
trying to point at, but I think the above is pointing at the '=' and
wants to say "auto_crlf variable is enum, it gets assigned an
integer returned from git_config_bool(), and I do not like that
assignment".

For this one I tend to agree with the compiler, meaning that it is
ugly to define "enum auto_crlf" in such a way that the first two
values happen to match what a logically different "enum" (which is
"boolean") assigns the two values to.  And this judgment does not
change whether git_config_bool() actually returns an enum or an int
(the code in reality returns the latter).

I do not think people would terribly mind a patch to turn the above
into:

  auto_crlf = git_config_bool(var, value) ? AUTO_CRLF_FALSE : AUTO_CRLF_TRUE;

> "/home/jenkins/.jenkins/workspace/Build_Git/config.c", line 1147:
> warning(272): 
>   enumerated type mixed with another type
>
> type = sha1_object_info(s->oid.hash, >size);
>^

/* returns enum object_type or negative */
int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)

The function has been like this forever, I suspect, and I would say
"this gives negative when error, or enum we know is non-negative" is
quite a reasonable thing to do, but the enum has OBJ_BAD defined to
be negative, so probably it is more kosher if sha1_object_info() is
declared to return "enum object_type" instead of int.

> "/home/jenkins/.jenkins/workspace/Build_Git/diff.c", line 3618:
> warning(272): 
>   enumerated type mixed with another type
>
> options->color_moved = diff_color_moved_default;
>^

If color_moved field is declared to be an enum, the _default
variable should be, too.  I do not think it is such a controversial
fix.

Re: [PATCH] Use MOVE_ARRAY

[Jeff King]

On Mon, Jan 22, 2018 at 03:26:59PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Most of these are "shift part of the array". I wonder if it would make
> > sense to encapsulate that pattern in a helper, like:
> >
> >   #define SHIFT_ARRAY(a, nr, pos, slots) \
> > MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots)
> >   ...
> >   SHIFT_ARRAY(it->down, it->subtree_nr, pos, 1);
> 
> Exactly my thought when I was queuing it, but I was wondering about
> this more from "can we use the higher level abstraction for reducing
> errors?" point of view.  If we are shifting an array by 3 slots to
> the right, we should at least have enough slots allocated to hold
> them (i.e. a->nr - a->alloc must be 3 or more).  But after realizing
> that the level these macros operate at is still a bit too low to do
> something like that, I quickly lost interest ;-)

Yeah, you'd need to know the "alloc" number to right-shift correctly,
since by definition we're pushing off the end of a->nr. Left-shifts just
need to make sure they don't go past "0", which we can do here, but I'd
think they're pretty uncommon.

The right macro level may actually be something more like "make room for
N items at pos". E.g.:

  #define CREATE_ARRAY_HOLE(a, nr, alloc, pos, slots) do { \
  ALLOC_GROW(a, nr + slots, alloc);
  MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots);
  } while (0)

but I didn't investigate the surrounding code. And it surely would need
a catchier name. ;)

-Peff

[PATCH] daemon: add --no-syslog to undo implicit --syslog

[Lucas Werkmeister]

Several options imply --syslog, without there being a way to disable it
again. This commit adds that option.

This is useful, for instance, when running `git daemon` as a systemd
service with --inetd. systemd connects stderr to the journal by default,
so logging to stderr is useful. On the other hand, log messages sent via
syslog also reach the journal eventually, but run the risk of being
processed at a time when the `git daemon` process has already exited
(especially if the process was very short-lived, e.g. due to client
error), so that the journal can no longer read its cgroup and attach the
message to the correct systemd unit. See systemd/systemd#2913 [1].

[1]: https://github.com/systemd/systemd/issues/2913

Signed-off-by: Lucas Werkmeister 
---

Notes:
I decided not to add the option to git-daemon's --help output, since
the similar --no-informative-errors option is also not listed there.
Let me know if it should be added.

Feel free to remove the part about systemd from the commit message
if you feel it doesn't need to be included.

 Documentation/git-daemon.txt | 6 --
 daemon.c | 4 
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3c91db7be..dfd6ce03c 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -8,7 +8,7 @@ git-daemon - A really simple server for Git repositories
 SYNOPSIS
 
 [verse]
-'git daemon' [--verbose] [--syslog] [--export-all]
+'git daemon' [--verbose] [--[no-]syslog] [--export-all]
 [--timeout=] [--init-timeout=] [--max-connections=]
 [--strict-paths] [--base-path=] [--base-path-relaxed]
 [--user-path | --user-path=]
@@ -109,9 +109,11 @@ OPTIONS
Maximum number of concurrent clients, defaults to 32.  Set it to
zero for no limit.
 
---syslog::
+--[no-]syslog::
Log to syslog instead of stderr. Note that this option does not imply
--verbose, thus by default only error conditions will be logged.
+   `--no-syslog` is the default, but may be given explicitly to override
+   the implicit `--syslog` of an earlier `--inetd` or `--detach` option.
 
 --user-path::
 --user-path=::
diff --git a/daemon.c b/daemon.c
index e37e343d0..d59fef6d6 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1300,6 +1300,10 @@ int cmd_main(int argc, const char **argv)
log_syslog = 1;
continue;
}
+   if (!strcmp(arg, "--no-syslog")) {
+   log_syslog = 0;
+   continue;
+   }
if (!strcmp(arg, "--export-all")) {
export_all_trees = 1;
continue;
-- 
2.16.0

Re: [PATCH] Use MOVE_ARRAY

[Junio C Hamano]

Jeff King  writes:

> Most of these are "shift part of the array". I wonder if it would make
> sense to encapsulate that pattern in a helper, like:
>
>   #define SHIFT_ARRAY(a, nr, pos, slots) \
> MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots)
>   ...
>   SHIFT_ARRAY(it->down, it->subtree_nr, pos, 1);

Exactly my thought when I was queuing it, but I was wondering about
this more from "can we use the higher level abstraction for reducing
errors?" point of view.  If we are shifting an array by 3 slots to
the right, we should at least have enough slots allocated to hold
them (i.e. a->nr - a->alloc must be 3 or more).  But after realizing
that the level these macros operate at is still a bit too low to do
something like that, I quickly lost interest ;-)

> I'm not sure if that's more readable because it describes a higher-level
> operation, or if it's less because it adds yet another non-standard
> helper for the reader to learn.

Yeah, conflicting goals.  It indeed is easier to see what is going
on when reading the code, once the reader gets used to them.

Thanks.

Re: [PATCH] Fix comma splices

[Junio C Hamano]

Jeff King  writes:

> (To be pedantic, these aren't comma splices. A comma splice joins two
> independent clauses with a comma and _without_ a conjunction).

Thanks for clearing up the "Huh?" I felt earlier when I threw the
patch to "to look at later" bin after finding updated text for most
of them read not much better than the original, at least to me.

So I am OK to take this patch (or an updated version), but the log
message needs updating, I guess.

>   2. Is this Q_() here actually helping? It triggers on "ours + theirs"
>   ...
>  In fact, I don't think the singular case here can _ever_ trigger,

Exactly.

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Patches or cover letters generated by format-patch are meant to be
> exchanged as emails, most of the time. And since it's generally agreed
> that text in mails should be wrapped around 70 columns or so, make sure
> these diffstat follow the convention.
>
> I noticed this when I quoted a diffstat line [1]. Should we do something
> like this? diffstat is rarely quoted though so perhaps the stat width
> should be something like 75.
>
> t4052 fails but I don't think it's worth fixing until it's clear if it's
> worth doing this.

I guess you meant this as an RFC/PATCH; FWIW, I personally am in
favor of this change.

>
> [1] https://public-inbox.org/git/20180122121426.GD5980@ash/T/#u
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  builtin/log.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 14fdf39165..6be79656c5 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int 
> use_stdout,
>  
>   memcpy(, >diffopt, sizeof(opts));
>   opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> + opts.diffopt.stat_width = 70;
>  
>   diff_setup_done();
>  
> @@ -1611,9 +1612,12 @@ int cmd_format_patch(int argc, const char **argv, 
> const char *prefix)
>   die(_("--check does not make sense"));
>  
>   if (!use_patch_format &&
> - (!rev.diffopt.output_format ||
> -  rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> + (!rev.diffopt.output_format ||
> +  rev.diffopt.output_format == DIFF_FORMAT_PATCH)) {
>   rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | 
> DIFF_FORMAT_SUMMARY;
> + if (!rev.diffopt.stat_width)
> + rev.diffopt.stat_width = 70;
> + }
>  
>   /* Always generate a patch */
>   rev.diffopt.output_format |= DIFF_FORMAT_PATCH;

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index af9b847761..d2a8e0312a 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -401,4 +401,23 @@ done <<\EOF
>  0642 -rw-r---w-
>  EOF
>  
> +test_expect_success SANITY 'graceful handling when splitting index is not 
> allowed' '

Is SANITY the only prereq we want, or do we want both it and POSIXPERM?

In "git grep SANITY t/" output, we see that they are almost always
used together.

> + test_create_repo ro &&
> + (
> + cd ro &&
> + test_commit initial &&
> + git update-index --split-index &&
> + test -f .git/sharedindex.*
> + ) &&
> + cp ro/.git/index new-index &&
> + test_when_finished "chmod u+w ro/.git" &&
> + chmod u-w ro/.git &&
> + GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index 
> &&
> + chmod u+w ro/.git &&
> + rm ro/.git/sharedindex.* &&
> + GIT_INDEX_FILE=new-index git ls-files >actual &&
> + echo initial.t >expected &&
> + test_cmp expected actual
> +'
> +
>  test_done

Re: [PATCH] Fix comma splices

[Jeff King]

On Sun, Jan 21, 2018 at 10:19:04PM -0600, fel...@felipegasper.com wrote:

> Subject: [PATCH] Fix comma splices in remote.c
> [...]
> @@ -2123,9 +2123,9 @@ int format_tracking_info(struct branch *branch, struct 
> strbuf *sb)
>   _("  (use \"git push\" to publish your local 
> commits)\n"));
>   } else if (!ours) {
>   strbuf_addf(sb,
> - Q_("Your branch is behind '%s' by %d commit, "
> + Q_("Your branch is behind '%s' by %d commit "
>  "and can be fast-forwarded.\n",
> -"Your branch is behind '%s' by %d commits, "
> +"Your branch is behind '%s' by %d commits "
>  "and can be fast-forwarded.\n",

Yes, the original violates the usual English rules for commas, which say
that the dependent clause joined with a conjunction should not have a
comma.

(To be pedantic, these aren't comma splices. A comma splice joins two
independent clauses with a comma and _without_ a conjunction).

This kind of comma _can_ actually be considered correct if it makes the
sentence more clear, or to indicate a more extreme contrast. I tend to
agree with you, though, that it does not help clarity here at all.

> @@ -2134,11 +2134,11 @@ int format_tracking_info(struct branch *branch, 
> struct strbuf *sb)
>   _("  (use \"git pull\" to update your local 
> branch)\n"));
>   } else {
>   strbuf_addf(sb,
> - Q_("Your branch and '%s' have diverged,\n"
> -"and have %d and %d different commit each, "
> + Q_("Your branch and '%s' have diverged.\n"
> +"They have %d and %d different commit each, "
>  "respectively.\n",

This one is the same case as above, but you solved it differently. I
agree that the result reads more clearly than the original. And more
clearly than:

  Your branch and '%s' have diverged and have %d and %d different
  commits each, respectively.

The first hunk could use the same "start a new sentence" trick, but it's
pretty clear as a single sentence.  So I think your patch is doing the
right thing, my pedantic explanations notwithstanding. ;)

Reading this did make me wonder two things, though:

  1. Do we really need the newline after "diverged"? Especially with the
 comma, it makes the result look funnily wrapped (unless you have an
 extremely long upstream branch name).

 OTOH, if we switch it to a period as your patch does, I think the
 line break looks much more natural.

  2. Is this Q_() here actually helping? It triggers on "ours + theirs"
 being greater than 1. So the "singular" case could only come up if
 you had "0" in one of the cases.  But:

   ...and have 0 and 1 different commit each...

 makes no sense to me in English. It would still be "commits". I
 could accept:

   ...and have 1 commit each...

 for the case where it's 1/1. But we would use "1 and 1 different
 commits each" (which is correct, albeit slightly clunkier).

 In fact, I don't think the singular case here can _ever_ trigger,
 because this is the "else" block we get to when we know that
 neither is zero. So I think the singular half of this Q_() could
 just go away.

 Like:

diff --git a/remote.c b/remote.c
index 4c84ba88dc..52672ac658 100644
--- a/remote.c
+++ b/remote.c
@@ -2096,13 +2096,9 @@ int format_tracking_info(struct branch *branch, struct 
strbuf *sb)
_("  (use \"git pull\" to update your local 
branch)\n"));
} else {
strbuf_addf(sb,
-   Q_("Your branch and '%s' have diverged,\n"
-  "and have %d and %d different commit each, "
-  "respectively.\n",
-  "Your branch and '%s' have diverged,\n"
-  "and have %d and %d different commits each, "
-  "respectively.\n",
-  ours + theirs),
+   _("Your branch and '%s' have diverged,\n"
+ "and have %d and %d different commits each, "
+ "respectively.\n"),
base, ours, theirs);
if (advice_status_hints)
strbuf_addstr(sb,

(if I were doing such a series, I'd probably do that first as a
preparatory step, and then do grammatical fix on top).

-Peff

Re: [PATCH v2 07/12] git remote doc: correct dangerous lies about what prune does

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 22 2018, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> +Deletes stale references associated with . By default stale
>> +remote-tracking branches under , but depending on global
>> +configuration and the configuration of the remote we might even prune
>> +local tags
>
> An optional clarification
>
> s/prune local tags/& that haven't been pushed there/
>
>> +... Equivalent to `git fetch  --prune`, except that no
>> +new references will be fetched.
>
> `git fetch --prune ` (dashed options first and then non
> options), just like you wrote in the previous step.

Thanks. Will fix both.

Re: [PATCH v2 04/12] fetch tests: double quote a variable for interpolation

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 22 2018, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> If the $cmdline variable contains multiple arguments they won't be
>> interpolated correctly since the body of the test is single quoted. I
>> don't know what part of test-lib.sh is expanding variables within
>> single-quoted strings,...
>
> dothis='echo whatever $IFS separated strings'
> test_expect_success label '
> $dothis
> '
>
> works because test_expect_success ends up beint a glorified 'eval'
> and it sees the value of $dothis.
>
>> but interpolating this inline is the desired
>> behavior here.
>
> I am not sure what you meant by this, though.

Looking into this again:

myvar="a b 'c' \"d\" \"\\\"e\\\"\""
test_expect_success 'blah' '
/usr/bin/perl -MData::Dumper -wE say\ Dumper\ \\@ARGV -- $myvar &&
/usr/bin/perl -MData::Dumper -wE say\ Dumper\ \\@ARGV -- '"$myvar"' &&
echo $myvar &&
false
'

Produces:

Initialized empty Git repository in /home/avar/g/git/t/trash 
directory.t5510-fetch/.git/
expecting success:
/usr/bin/perl -MData::Dumper -wE say\ Dumper\ \\@ARGV -- $myvar &&
/usr/bin/perl -MData::Dumper -wE say\ Dumper\ \\@ARGV -- a b 'c' "d" 
"\"e\"" &&
echo $myvar &&
false

+ /usr/bin/perl -MData::Dumper -wE say Dumper \@ARGV -- a b 'c' "d" "\"e\""
$VAR1 = [
  'a',
  'b',
  '\'c\'',
  '"d"',
  '"\\"e\\""'
];

+ /usr/bin/perl -MData::Dumper -wE say Dumper \@ARGV -- a b c d "e"
$VAR1 = [
  'a',
  'b',
  'c',
  'd',
  '"e"'
];

+ echo a b 'c' "d" "\"e\""
a b 'c' "d" "\"e\""
+ false
error: last command exited with $?=1
not ok 1 - blah
#
#   /usr/bin/perl -MData::Dumper -wE say\ Dumper\ \\@ARGV -- $myvar 
&&
#   /usr/bin/perl -MData::Dumper -wE say\ Dumper\ \\@ARGV -- a b 
'c' "d" "\"e\"" &&
#   echo $myvar &&
#   false
#

I.e. the desired effect is to get variables like the refspec like/this
not 'like/this'. I could also just apply this on top, which gives the
same end result, but now I wonder if starting some args with
e.g. unescaped + and with : and * in the string is portable:

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 48f49b613a..2d311059e9 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -587 +587 @@ test_configured_prune () {
-   git fetch '"$cmdline"' &&
+   git fetch $cmdline &&
@@ -621 +621 @@ test_configured_prune unset unset unset unset kept   pruned 
\
-   "--prune origin 'refs/tags/*:refs/tags/*'"
+   "--prune origin refs/tags/*:refs/tags/*"
@@ -623 +623 @@ test_configured_prune unset unset unset unset pruned pruned 
\
-   "--prune origin 'refs/tags/*:refs/tags/*' 
'+refs/heads/*:refs/remotes/origin/*'"
+   "--prune origin refs/tags/*:refs/tags/* 
+refs/heads/*:refs/remotes/origin/*"
@@ -641 +641 @@ test_configured_prune false false unset unset kept   pruned 
\
-   "--prune origin 'refs/tags/*:refs/tags/*'"
+   "--prune origin refs/tags/*:refs/tags/*"
@@ -643 +643 @@ test_configured_prune false false unset unset pruned pruned 
\
-   "--prune origin 'refs/tags/*:refs/tags/*' 
'+refs/heads/*:refs/remotes/origin/*'"
+   "--prune origin refs/tags/*:refs/tags/* 
+refs/heads/*:refs/remotes/origin/*"
@@ -661 +661 @@ test_configured_prune true  true  unset unset kept   pruned 
\
-   "--prune origin 'refs/tags/*:refs/tags/*'"
+   "--prune origin refs/tags/*:refs/tags/*"
@@ -663 +663 @@ test_configured_prune true  true  unset unset pruned pruned 
\
-   "--prune origin 'refs/tags/*:refs/tags/*' 
'+refs/heads/*:refs/remotes/origin/*'"
+   "--prune origin refs/tags/*:refs/tags/* 
+refs/heads/*:refs/remotes/origin/*"
@@ -684 +684 @@ test_configured_prune true  false true  false kept   kept   
""
-# When --prune-tags is supplied it's ignored if an explict refspec is
+# When --prune-tags is supplied its ignored if an explict refspec is
@@ -687 +687 @@ test_configured_prune unset unset unset unset pruned kept \
-   "--prune --prune-tags origin '+refs/heads/*:refs/remotes/origin/*'"
+   "--prune --prune-tags origin +refs/heads/*:refs/remotes/origin/*"
@@ -689 +689 @@ test_configured_prune unset unset true  unset pruned kept \
-   "--prune --prune-tags origin '+refs/heads/*:refs/remotes/origin/*'"
+   "--prune --prune-tags origin +refs/heads/*:refs/remotes/origin/*"
@@ -691 +691 @@ test_configured_prune unset unset unset true pruned  kept \
-   "--prune --prune-tags origin '+refs/heads/*:refs/remotes/origin/*'"
+   "--prune --prune-tags origin +refs/heads/*:refs/remotes/origin/*"


>> -git fetch $cmdline &&
>> +git fetch 

RE: [Nit] Lots of enumerated type warnings

[Randall S. Becker]

On January 22, 2018 5:41 PM, Junio C Hamano wrote:
> "Randall S. Becker"  writes:
> 
> > I'm seeing an increase in the enumerated type warnings coming from my
> > use of the c99 compiler for compiling git over time (loads more for
> > 2.16.0 compared to 2.3.7 when I took it on).
> 
> What exactly do these "warnings" complain about?  Without knowing that,
> the remainder of your question cannot be answered.
> 
> Does it complain against enum FOO {A,B,C,} saying that the comma after C
is
> not kosher in older C standard, for example?

Here are a few examples, there are more:

auto_crlf = git_config_bool(var, value);
  ^
"/home/jenkins/.jenkins/workspace/Build_Git/config.c", line 1147:
warning(272): 
  enumerated type mixed with another type

type = sha1_object_info(s->oid.hash, >size);
 ^
"/home/jenkins/.jenkins/workspace/Build_Git/diff.c", line 3618:
warning(272): 
  enumerated type mixed with another type

options->color_moved = diff_color_moved_default;
 ^
"/home/jenkins/.jenkins/workspace/Build_Git/diff.c", line 4108:
warning(272): 
  enumerated type mixed with another type

options->color_moved = 0;
 ^
"/home/jenkins/.jenkins/workspace/Build_Git/diff.c", line 4218:
warning(272): 
  enumerated type mixed with another type

Re: [PATCH] Use MOVE_ARRAY

[Jeff King]

On Mon, Jan 22, 2018 at 06:50:09PM +0100, SZEDER Gábor wrote:

> Use the helper macro MOVE_ARRAY to move arrays.  This is shorter and
> safer, as it automatically infers the size of elements.
> 
> Patch generated by Coccinelle and contrib/coccinelle/array.cocci in
> Travis CI's static analysis build job.

Seems pretty straightforward. One thing I did notice...

> diff --git a/cache-tree.c b/cache-tree.c
> index e03e72c34a..18946aa458 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -84,9 +84,8 @@ static struct cache_tree_sub *find_subtree(struct 
> cache_tree *it,
>   down->namelen = pathlen;
>  
>   if (pos < it->subtree_nr)
> - memmove(it->down + pos + 1,
> - it->down + pos,
> - sizeof(down) * (it->subtree_nr - pos - 1));
> + MOVE_ARRAY(it->down + pos + 1, it->down + pos,
> +it->subtree_nr - pos - 1);

Most of these are "shift part of the array". I wonder if it would make
sense to encapsulate that pattern in a helper, like:

  #define SHIFT_ARRAY(a, nr, pos, slots) \
MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots)
  ...
  SHIFT_ARRAY(it->down, it->subtree_nr, pos, 1);

I'm not sure if that's more readable because it describes a higher-level
operation, or if it's less because it adds yet another non-standard
helper for the reader to learn.

-Peff

RE: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Randall S. Becker]

On January 22, 2018 5:36 PM, Junio C Hamano wrote:
> Torsten Bögershausen  writes:
> 
> > On Fri, Jan 19, 2018 at 12:33:59PM -0500, randall.s.bec...@rogers.com
> wrote:
> >> From: "Randall S. Becker" 
> >>
> >> * wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
> >>   default on the NonStop platform.
> >>
> >> Signed-off-by: Randall S. Becker 
> >> ---
> >>  wrapper.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/wrapper.c b/wrapper.c
> >> index d20356a77..671cbb4b4 100644
> >> --- a/wrapper.c
> >> +++ b/wrapper.c
> >> @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
> >>FILE *stream = fdopen(fd, mode);
> >>if (stream == NULL)
> >>die_errno("Out of memory? fdopen failed");
> >> +#ifdef __TANDEM
> >> +  setbuf(stream,0);
> >> +#endif
> >
> > Reading the commit message, I would have expected someting similar to
> >
> > #ifdef FORCE_PIPE_FLUSHES
> > setbuf(stream,0);
> > #endif
> >
> > (Because other systems may need the tweak as well, some day) Of course
> > you need to change that in the Makefile and config.mak.uname
> 
> I actually wouldn't have expected anything like that after reading the commit
> message.
> 
> First I thought it was describing only what it does (i.e. "let's use
> setbuf() to set the stream unbuffered on TANDEM"), which is a useless
> description that only says what it does which we can read from the diff, but
> "NonStop by default creates pipe that does not flush" is a potentially useful
> information the log message adds.
> But it is just "potentially"---we cannot read what exact problem the change is
> trying to address.  Making a pipe totally unbuffered is a heavy hammer that
> may not be an appropriate solution---it could be that we are missing calls to
> fflush() where we need and have been lucky because most of the systems
> we deal with do line-buffered by default, or something silly/implausible like
> that, and if that is the case, a more proper fix may be to add these missing
> fflush() to right places.
> 
> IOW, I do not see it explained clearly why this change is needed on any single
> platform---so "that issue may be shared by others, too"
> is a bit premature thing for me to listen to and understand, as "that issue" 
> is
> quite unclear to me.

v4 might be a little better. The issue seems to be specific to NonStop that 
it's PIPE mechanism needs to have setbuf(pipe,NULL) called for git to be happy. 
The default behaviour appears to be different on NonStop from other platforms 
from our testing. We get hung up waiting on pipes unless this is done. At the 
moment, this is platform-specific. Other parts of the discussion led to the 
conclusion that we should make this available to any platform using a new 
configuration option, but my objective is to get the NonStop port integrated 
with the main git code base and when my $DAYJOB permits it, spend the time 
adding the option. Note: __TANDEM is #define automatically emitted by the 
NonStop compilers. 

Does that help?

Sincerely,
Randall

Re: [Nit] Lots of enumerated type warnings

[Junio C Hamano]

"Randall S. Becker"  writes:

> I'm seeing an increase in the enumerated type warnings
> coming from my use of the c99 compiler for compiling git over time (loads
> more for 2.16.0 compared to 2.3.7 when I took it on).

What exactly do these "warnings" complain about?  Without knowing
that, the remainder of your question cannot be answered.

Does it complain against enum FOO {A,B,C,} saying that the comma
after C is not kosher in older C standard, for example?

Re: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform

[Junio C Hamano]

Torsten Bögershausen  writes:

> On Fri, Jan 19, 2018 at 12:33:59PM -0500, randall.s.bec...@rogers.com wrote:
>> From: "Randall S. Becker" 
>> 
>> * wrapper.c: called setbuf(stream,0) to force pipe flushes not enabled by
>>   default on the NonStop platform.
>> 
>> Signed-off-by: Randall S. Becker 
>> ---
>>  wrapper.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/wrapper.c b/wrapper.c
>> index d20356a77..671cbb4b4 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -403,6 +403,9 @@ FILE *xfdopen(int fd, const char *mode)
>>  FILE *stream = fdopen(fd, mode);
>>  if (stream == NULL)
>>  die_errno("Out of memory? fdopen failed");
>> +#ifdef __TANDEM
>> +setbuf(stream,0);
>> +#endif
>
> Reading the commit message, I would have expected someting similar to
>
> #ifdef FORCE_PIPE_FLUSHES
>   setbuf(stream,0);
> #endif
>
> (Because other systems may need the tweak as well, some day)
> Of course you need to change that in the Makefile and config.mak.uname

I actually wouldn't have expected anything like that after reading
the commit message.  

First I thought it was describing only what it does (i.e. "let's use
setbuf() to set the stream unbuffered on TANDEM"), which is a
useless description that only says what it does which we can read
from the diff, but "NonStop by default creates pipe that does not
flush" is a potentially useful information the log message adds.
But it is just "potentially"---we cannot read what exact problem the
change is trying to address.  Making a pipe totally unbuffered is a
heavy hammer that may not be an appropriate solution---it could be
that we are missing calls to fflush() where we need and have been
lucky because most of the systems we deal with do line-buffered by
default, or something silly/implausible like that, and if that is
the case, a more proper fix may be to add these missing fflush() to
right places.

IOW, I do not see it explained clearly why this change is needed on
any single platform---so "that issue may be shared by others, too"
is a bit premature thing for me to listen to and understand, as
"that issue" is quite unclear to me.

Re: [PATCH 2/8] sequencer: introduce the `merge` command

[Junio C Hamano]

Johannes Schindelin  writes:

>   end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> + item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> + item->arg_len = (int)(eol - item->arg);
> +
>   saved = *end_of_object_name;
> + if (item->command == TODO_MERGE && *bol == '-' &&
> + bol + 1 == end_of_object_name) {
> + item->commit = NULL;
> + return 0;
> + }
> +
>   *end_of_object_name = '\0';
>   status = get_oid(bol, _oid);
>   *end_of_object_name = saved;
>  
> - item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> - item->arg_len = (int)(eol - item->arg);
> -

Assigning to "saved" before the added "if we are doing merge and see
'-', do this special thing" is not only unnecessary, but makes the
logic in the non-special case harder to read.  The four things
"saved = *eol; *eol = 0; do_thing_using(bol); *eol = saved;" is a
single logical unit; keep them together.

This hunk may have been the most expedient way to coax "-" into the
location where a commit object name is expected; it looks ugly, but
for the limited purpose of this series it should do.

> @@ -2069,6 +2077,132 @@ static int do_reset(const char *name, int len)
>   return ret;
>  }
>  
> +static int do_merge(struct commit *commit, const char *arg, int arg_len,
> + struct replay_opts *opts)
> +{
> + int merge_arg_len;
> + struct strbuf ref_name = STRBUF_INIT;
> + struct commit *head_commit, *merge_commit, *i;
> + struct commit_list *common, *j, *reversed = NULL;
> + struct merge_options o;
> + int ret;
> + static struct lock_file lock;
> +
> + for (merge_arg_len = 0; merge_arg_len < arg_len; merge_arg_len++)
> + if (isspace(arg[merge_arg_len]))
> + break;

Mental note: this scans for a whitespace, and tab is accepted
instead of SP, which presumably is to allow human typed string.

> + if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
> + return -1;
> +
> + if (commit) {
> + const char *message = get_commit_buffer(commit, NULL);
> + const char *body;
> + int len;
> +
> + if (!message) {
> + rollback_lock_file();
> + return error(_("could not get commit message of '%s'"),
> +  oid_to_hex(>object.oid));
> + }
> + write_author_script(message);
> + find_commit_subject(message, );
> + len = strlen(body);
> + if (write_message(body, len, git_path_merge_msg(), 0) < 0) {
> + error_errno(_("Could not write '%s'"),
> + git_path_merge_msg());
> + unuse_commit_buffer(commit, message);
> + rollback_lock_file();
> + return -1;
> + }
> + unuse_commit_buffer(commit, message);
> + } else {
> + const char *p = arg + merge_arg_len;
> + struct strbuf buf = STRBUF_INIT;
> + int len;
> +
> + strbuf_addf(, "author %s", git_author_info(0));
> + write_author_script(buf.buf);
> + strbuf_reset();
> +
> + p += strspn(p, " \t");

... and this matches the above mental note.  It allows consecutive
whitespaces as a separator, which is sensible behaviour.

> + if (*p)
> + len = strlen(p);
> + else {
> + strbuf_addf(, "Merge branch '%.*s'",
> + merge_arg_len, arg);
> + p = buf.buf;
> + len = buf.len;
> + }

So... "arg" received by this function can be a single non-whitespace
token, which is taken as the name of the branch being merged (in
this else clause).  Or it can also be followed by a single liner
message for the merge commit.  Presumably, this is for creating a
new merge (i.e. "commit==NULL" case), and preparing a proper log
message in the todo list is unrealistic, so this would be a
reasonable compromise.  Those users who want to write proper log
message could presumably follow such "merge" insn with a "x git
commit --amend" or something, I presume, if they really wanted to.

> + if (write_message(p, len, git_path_merge_msg(), 0) < 0) {
> + error_errno(_("Could not write '%s'"),
> + git_path_merge_msg());
> + strbuf_release();
> + rollback_lock_file();
> + return -1;
> + }
> + strbuf_release();
> + }

OK.  Now we have prepared the MERGE_MSG file and are ready to commit.

> + head_commit = lookup_commit_reference_by_name("HEAD");
> + if (!head_commit) {
> + rollback_lock_file();
> + return error(_("Cannot 

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[SZEDER Gábor]

On Mon, Jan 22, 2018 at 8:46 PM, Eric Sunshine  wrote:
> On Mon, Jan 22, 2018 at 1:27 PM, SZEDER Gábor  wrote:

>>   - The logs of OSX build jobs have CRCRLF line endings.  However, the
>> 'base64' util of OSX doesn't wrap its output at 76 columns, i.e.
>> prints one single albeit very long line.  This means that while
>
> Perhaps you could pipe the 'base64' output through 'fold' or 'fmt'?

No need to, according to its manpage[1], OSX's base64 has an option to
wrap lines after given number of characters.  Of course it uses a
different letter for the option than the coreutils base64... :)

(While long lines are ugly, in this particular case it comes handy: when
using vim to extract the base64-encoded section from the log to a
separate file, it's less keystrokes to yank a single line than to search
for the end of the encoded section.)


[1] - 
https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man1/base64.1.html

Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision

[Junio C Hamano]

Jacob Keller  writes:

> The code looks good, but I'm a little wary of adding bud which
> hard-codes a specific label. I suppose it does grant a bit of
> readability to the resulting script... ? It doesn't seem that
> important compared to use using "reset onto"? At least when
> documenting this it should be made clear that the "onto" label is
> special.

I do not think we would mind "bud" too much in the end result, but
the change in 1/8 is made harder to read than necessary with it.  It
is the only thing that needs "a single-letter command name may now
not have any argument after it" change to the parser among the three
things being added here, and it also needs to be added to the list
of special commands without arguments.

It would have been easier to reason about if addition of "bud" was
in its own patch done after label and reset are added.  And if done
as a separate step, perhaps it would have been easier to realize
that it would be a more future-proof solution for handling the
"special" ness of BUD to add a new "unsigned flags" word to
todo_command_info[] structure and using a bit that says "this does
not take an arg" than to hardcode "noop and bud are the commands
without args" in the code.  That hardcode was good enough when there
was only one thing in that special case.  Now it has two.

In a similar way, the code to special case label and reset just like
exec may also want to become more table driven, perhaps using
another bit in the same new flags word to say "this does not refer
to commit".  I think that can become [v2 1/N] while addition of "bud"
can be [v2 2/N] (after all, "bud" just does the same do_reset() with
hardcoded argument, so "label/reset" must come first).

Re: [PATCH v2 12/12] fetch: add a --fetch-prune option and fetch.pruneTags config

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Add a --fetch-prune option to git-fetch, along with fetch.pruneTags
> config option. This allows for doing any of:
>
> git fetch -p -P
> git fetch --prune --prune-tags
> git fetch -p -P origin
> git fetch --prune --prune-tags origin
>
> Or simply:
>
> git config fetch.prune true &&
> git config fetch.pruneTags true &&
> git fetch
>
> Instead of the much more verbose:
>
> git fetch --prune origin 'refs/tags/*:refs/tags/*' 
> '+refs/heads/*:refs/remotes/origin/*'
>
> Before this feature it was painful to support the use-case of pulling
> from a repo which is having both its branches *and* tags deleted
> regularly, and wanting our local references to reflect upstream.
>
> At work we create deployment tags in the repo for each rollout, and
> there's *lots* of those, so they're archived within weeks for
> performance reasons.
>
> Without this change it's hard to centrally configure such repos in
> /etc/gitconfig (on servers that are only used for working with
> them). You need to set fetch.prune=true globally, and then for each
> repo:
>
> git -C {} config --replace-all remote.origin.fetch 
> "refs/tags/*:refs/tags/*" "^refs/tags/*:refs/tags/*$"

I think the last one is supposed to be a regular expression on
existing values.  Shouldn't the asterisks be quoted?  

Otherwise, it would appears as if "refs/tags:refs/tags///" are
replaced with "refs/tags/*:refs/tags/*", but it certainly is not
what you are doing.  I also wonder why the existing one does not
expect a leading '+', which I think is what we place by default
when you clone.

> +-P::
> +--prune-tags::
> +  This option is
> + merely a shorthand for providing the explicit tag refspec
> + along with `--prune`, see the discussion about that in its
> + documentation.

So would "git fetch -P origin" be like "git fetch --prune --tags
origin"?

>  +
>  See the PRUNING section below for more details.
>  
> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> index 18fac0da2e..5682ed4ae1 100644
> --- a/Documentation/git-fetch.txt
> +++ b/Documentation/git-fetch.txt
> @@ -148,6 +148,30 @@ So be careful when using this with a refspec like
>  `refs/tags/*:refs/tags/*`, or any other refspec which might map
>  references from multiple remotes to the same local namespace.
>  
> +Since keeping up-to-date with both branches and tags on the remote is
> +a common use-case the `--prune-tags` option can be supplied along with
> +`--prune` to prune local tags that don't exist on the remote. Tag
> +pruning can also be enabled with `fetch.pruneTags` or
> +`remote..pruneTags` in the config. See linkgit:git-config[1].
> +
> +The `--prune-tags` option is equivalent to having
> +`refs/tags/*:refs/tags/*` configured in the refspecs for the
> +remote. This can lead to some seemingly strange interactions:
> +
> +
> +# These both fetch tags
> +$ git fetch --no-tags origin 'refs/tags/*:refs/tags/*'
> +$ git fetch --no-tags --prune-tags origin
> +

This description is too confusing.  First you say "having
... configured in the refspecs for the remote", but configured
refspecs for the remote (I presume that you are missing 'fetch' from
that description and are talking about the "remote.$name.fetch"
configuration variable) are overridden when you give explicit
refspecs from the command line, so the above two are not even
equivalent.  The first one gives a refspec explicitly from the
command line, so other configured refspecs like

[remote "origin"] fetch = +refs/heads/*:refs/remotes/origin/*

should be ignored, while the second one, if --prune-tags tells Git
to behave as if 

[remote "origin"] fetch = refs/tags/*:refs/tags/*

also exists in the config, would not cause other ones for the same
remote from getting ignored.  So...

RE

[Mr Sheng Li Hung]

I am Mr.Sheng Li Hung, from china I got your information while search for
a reliable person, I have a very profitable business proposition for you
and i can assure you that you will not regret been part of this mutual
beneficial transaction after completion. Kindly get back to me for more
details on this email id: shengli...@outlook.com

Thanks
Mr Sheng Li Hung

Re: [PATCH] worktree: teach "add" to check out existing branches

[Thomas Gummerer]

On 01/22, Duy Nguyen wrote:
> On Sun, Jan 21, 2018 at 7:02 PM, Thomas Gummerer  wrote:
> > [...]
> >  +
> >  If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
> > -then, as a convenience, a new branch based at HEAD is created 
> > automatically,
> > -as if `-b $(basename )` was specified.
> > +then, as a convenience, a worktree with a branch named after
> > +`$(basename )` (call it ``) is created.  If ``
> > +doesn't exist, a new branch based on HEAD is automatically created as
> > +if `-b ` was given.  If `` exists in the repository,
> > +it will be checked out in the new worktree, if it's not checked out
> > +anywhere else, otherwise the command will refuse to create the
> > +worktree.
> 
> It starts getting a bit too magical to me. We probably should print
> something "switching to branch..." or "creating new branch ..."  to
> let people know what decision was taken, unless --quiet is given. Yeah
> I know --quiet does not exist. You don't need to add it now either
> since nobody has asked for it.

I think that's a good idea.  I'll add that, thanks.

> More or less related to this, there was a question [1] whether we
> could do better than $(basename ) for determining branch name.
> Since you're doing start to check if a branch exists, people may start
> asking to check for branch "foo/bar" from the path abc/foo/bar instead
> of just branch "bar".

Thanks for pointing me at this.  I feel like we might get ourselves
some backwards compatibility worries when doing that.  Lets say
someone has a branch 'feature/a', using 'git worktree feature/a' would
currently create a new branch 'a', and does not die.

I'd guess most users would want 'feature/a' checked out in that case,
but we can't exactly be sure we won't break anyones workflow here.
With your suggestion I guess that would be mitigated somehow as it
shows which action is taken, but dunno.

Should we hide this behaviour behind a flag, and plan for it
eventually becoming the default?

> [1] https://github.com/git-for-windows/git/issues/1390
> 
> >
> >  list::
> >
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index 7cef5b120b..148a864bb9 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -411,13 +411,21 @@ static int add(int ac, const char **av, const char 
> > *prefix)
> > if (ac < 2 && !opts.new_branch && !opts.detach) {
> > int n;
> > const char *s = worktree_basename(path, );
> > -   opts.new_branch = xstrndup(s, n);
> > -   if (guess_remote) {
> > -   struct object_id oid;
> > -   const char *remote =
> > -   unique_tracking_name(opts.new_branch, );
> > -   if (remote)
> > -   branch = remote;
> > +   const char *branchname = xstrndup(s, n);
> > +   struct strbuf ref = STRBUF_INIT;
> 
> Perhaps a blank line after this to separate var declarations and the rest.

Will add. 

> > +   if (!strbuf_check_branch_ref(, branchname) &&
> > +   ref_exists(ref.buf)) {
> > +   branch = branchname;
> 
> Hmm.. do we need UNLEAK(branch);? Maybe you should try valgrind, I'm not sure.

Good question, I'll have a look, thanks.

> > +   opts.checkout = 1;
> > +   } else {
> > +   opts.new_branch = branchname;
> > +   if (guess_remote) {
> > +   struct object_id oid;
> > +   const char *remote =
> > +   
> > unique_tracking_name(opts.new_branch, );
> > +   if (remote)
> > +   branch = remote;
> > +   }
> > }
> > }
> >
> > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> > index 2b95944973..721b0e4c26 100755
> > --- a/t/t2025-worktree-add.sh
> > +++ b/t/t2025-worktree-add.sh
> > @@ -198,13 +198,22 @@ test_expect_success '"add" with  omitted' '
> > test_cmp_rev HEAD bat
> >  '
> >
> > -test_expect_success '"add" auto-vivify does not clobber existing branch' '
> > +test_expect_success '"add" auto-vivify checks out existing branch' '
> > test_commit c1 &&
> > test_commit c2 &&
> > git branch precious HEAD~1 &&
> > -   test_must_fail git worktree add precious &&
> > +   git worktree add precious &&
> > test_cmp_rev HEAD~1 precious &&
> > -   test_path_is_missing precious
> > +   (
> > +   cd precious &&
> > +   test_cmp_rev precious HEAD
> > +   )
> > +'
> > +
> > +test_expect_success '"add" auto-vivify fails with checked out branch' '
> > +   git checkout -b test-branch &&
> > +   test_must_fail git worktree add test-branch &&
> > +   test_path_is_missing test-branch
> >  '
> >
> 

Re: [PATCH v2 07/12] git remote doc: correct dangerous lies about what prune does

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> +Deletes stale references associated with . By default stale
> +remote-tracking branches under , but depending on global
> +configuration and the configuration of the remote we might even prune
> +local tags

An optional clarification

s/prune local tags/& that haven't been pushed there/

> +... Equivalent to `git fetch  --prune`, except that no
> +new references will be fetched.

`git fetch --prune ` (dashed options first and then non
options), just like you wrote in the previous step.

Re: [PATCH v2 04/12] fetch tests: double quote a variable for interpolation

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> If the $cmdline variable contains multiple arguments they won't be
> interpolated correctly since the body of the test is single quoted. I
> don't know what part of test-lib.sh is expanding variables within
> single-quoted strings,...

dothis='echo whatever $IFS separated strings'
test_expect_success label '
$dothis
'

works because test_expect_success ends up beint a glorified 'eval'
and it sees the value of $dothis.  

> but interpolating this inline is the desired
> behavior here.

I am not sure what you meant by this, though.

> - git fetch $cmdline &&
> + git fetch '"$cmdline"' &&

Would this work with cmdline that needs to be quoted for the
resulting shell script to be syntactically correct (e.g. cmdline
with a single dq in it)?  By stepping out of sq pair, you are
allowing/asking the shell that forms test_expect_success command
line arguments to interpolate cmdline, instead of asking the shell
that evals test_expect_success with its command line argument
strings.

In other words, I suspect that the caller of test_configured_prune
now must sq_quote the cmdline arguments it passes to this helper,
i.e.

cmdline="$(git rev-parse --sq-quote arg1 'arg"2' arg3)"
test_configured_prune ... "$cmdline" ...

for this patch to be correct.

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Eric Sunshine]

On Mon, Jan 22, 2018 at 1:27 PM, SZEDER Gábor  wrote:
> Subject: [PATCH] travis-ci: include the trash directories of failed tests in
>  the trace log
>
> The trash directory of a failed test might contain valuable
> information about the cause of the failure, but we have no access to
> the trash directories of Travis CI build jobs.  The only feedback we
> get from there is the trace log, so...
>
> Modify 'ci/print-test-failures.sh' to create a tar.gz archive of the
> test directory of each failed test and encode it with base64, so the
> result is a block of ASCII text that can be safely included in the
> trace log, along with a hint about how to restore it.  Furthermore,
> run tests with '--immediate' to faithfully preserve the failed state.
>
> A few of our tests create a sizeable trash directory, so limit the
> size of each included base64-encoded block, let's say, to 1MB.
>
> Note:
>
>   - The logs of Linux build jobs coming from Travis CI have mostly
> CRLF line endings, which makes 'base64 -d' from 'coreutils'
> complain about "invalid input"; it has to be converted to LF
> first.  'openssl base64 -d' can grok it just fine, even without
> conversion.
>
>   - The logs of OSX build jobs have CRCRLF line endings.  However, the
> 'base64' util of OSX doesn't wrap its output at 76 columns, i.e.
> prints one single albeit very long line.  This means that while

Perhaps you could pipe the 'base64' output through 'fold' or 'fmt'?

> 'base64' from 'coreutils' still complains, by the time it gets to
> the invalid input it already decoded everything and produced a
> valid .tar.gz.  OTOH, 'openssl base64 -d' doesn't grok it, but
> exits without any error message or even an error code, even after
> converting to CRLF or LF line endings.
>
> Go figure.
>
> Signed-off-by: SZEDER Gábor 

Re: SQUASH convert: add tracing for 'working-tree-encoding' attribute

[Eric Sunshine]

On Mon, Jan 22, 2018 at 1:00 PM,   wrote:
> diff --git a/convert.c b/convert.c
> @@ -1165,8 +1165,9 @@ static struct encoding *git_path_check_encoding(struct 
> attr_check_item *check)
> -   enc = xcalloc(1, sizeof(struct convert_driver));
> -   enc->name = xstrdup_toupper(value);  /* aways use upper case names! */
> +   enc = xcalloc(1, sizeof(*enc));
> +   /* Aways use upper case names to simplify subsequent string 
> comparison. */

s/Aways/Always/

https://public-inbox.org/git/2018012114.ga10...@ruderich.org/

> +   enc->name = xstrdup_toupper(value);
> *encoding_tail = enc;
> encoding_tail = &(enc->next);

Re: [PATCH v3] packed_ref_cache: don't use mmap() for small files

[Junio C Hamano]

Michael Haggerty  writes:

> `snapshot->buf` can still be NULL if the `packed-refs` file didn't exist
> (see the earlier code path in `load_contents()`). So either that code
> path *also* has to get the `xmalloc()` treatment, or my third patch is
> still necessary. (My second patch wouldn't be necessary because the
> ENOENT case makes `load_contents()` return 0, triggering the early exit
> from `create_snapshot()`.)
>
> I don't have a strong preference either way.

Which would be a two-liner, like the attached, which does not look
too bad by itself.

The direction, if we take this approach, means that we are declaring
that .buf being NULL is an invalid state for a snapshot to be in,
instead of saying "an empty snapshot looks exactly like one that was
freshly initialized", which seems to be the intention of the original
design.

After Kim's fix and with 3/3 in your follow-up series, various
helpers are still unsafe against .buf being NULL, like
sort_snapshot(), verify_buffer_safe(), clear_snapshot_buffer() (only
when mmapped bit is set), find_reference_location().

packed_ref_iterator_begin() checks if snapshot->buf is NULL and
returns early.  At the first glance, this appears a useful short cut
to optimize the empty case away, but the check also is acting as a
guard to prevent a snapshot with NULL .buf from being fed to an
unsafe find_reference_location().  An implicit guard like this feels
a bit more brittle than my liking.  If we ensure .buf is never NULL,
that check can become a pure short-cut optimization and stop being
a correctness thing.

So...


 refs/packed-backend.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index b6e2bc3c1d..1eeb5c7f80 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -473,12 +473,11 @@ static int load_contents(struct snapshot *snapshot)
if (fd < 0) {
if (errno == ENOENT) {
/*
-* This is OK; it just means that no
-* "packed-refs" file has been written yet,
-* which is equivalent to it being empty,
-* which is its state when initialized with
-* zeros.
+* Treat missing "packed-refs" as equivalent to
+* it being empty.
 */
+   snapshot->eof = snapshot->buf = xmalloc(0);
+   snapshot->mmapped = 0;
return 0;
} else {
die_errno("couldn't read %s", snapshot->refs->path);

Re: [PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[SZEDER Gábor]

On Thu, Jan 18, 2018 at 1:47 PM, Duy Nguyen  wrote:
> On Thu, Jan 18, 2018 at 6:36 PM, SZEDER Gábor  wrote:
>> This series, queued as 'nd/shared-index-fix', makes the 32 bit Linux
>> build job fail on Travis CI.  Unfortunately, all it can tell us about
>> the failure is this:
>>
>>   Test Summary Report
>>   ---
>>   t1700-split-index.sh (Wstat: 256 Tests: 23
>>   Failed: 1)
>> Failed test:  23
>> Non-zero exit status: 1
>>   Files=809, Tests=18491, 401 wallclock secs ( 7.22 usr  1.60 sys + 263.16
>>   cusr 49.58 csys = 321.56 CPU)
>>   Result: FAIL
>>
>> because it can't access the test's verbose log due to lack of
>> permissions.
>>
>>
>>   https://travis-ci.org/git/git/jobs/329681826#L2074
>
> I may need help getting that log (or even better the trash directory
> of t1700). 

Well, shower thoughts gave me an idea, see the included PoC patch below.
I can't really decide whether it's too clever or too ugly :)

It produces output like this (a previous WIP version without
'--immediate'):

  https://travis-ci.org/szeder/git/jobs/331601009#L2486


  -- >8 --

Subject: [PATCH] travis-ci: include the trash directories of failed tests in
 the trace log

The trash directory of a failed test might contain valuable
information about the cause of the failure, but we have no access to
the trash directories of Travis CI build jobs.  The only feedback we
get from there is the trace log, so...

Modify 'ci/print-test-failures.sh' to create a tar.gz archive of the
test directory of each failed test and encode it with base64, so the
result is a block of ASCII text that can be safely included in the
trace log, along with a hint about how to restore it.  Furthermore,
run tests with '--immediate' to faithfully preserve the failed state.

A few of our tests create a sizeable trash directory, so limit the
size of each included base64-encoded block, let's say, to 1MB.

Note:

  - The logs of Linux build jobs coming from Travis CI have mostly
CRLF line endings, which makes 'base64 -d' from 'coreutils'
complain about "invalid input"; it has to be converted to LF
first.  'openssl base64 -d' can grok it just fine, even without
conversion.

  - The logs of OSX build jobs have CRCRLF line endings.  However, the
'base64' util of OSX doesn't wrap its output at 76 columns, i.e.
prints one single albeit very long line.  This means that while
'base64' from 'coreutils' still complains, by the time it gets to
the invalid input it already decoded everything and produced a
valid .tar.gz.  OTOH, 'openssl base64 -d' doesn't grok it, but
exits without any error message or even an error code, even after
converting to CRLF or LF line endings.

Go figure.

Signed-off-by: SZEDER Gábor 
---
 ci/lib-travisci.sh|  2 +-
 ci/print-test-failures.sh | 20 
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 1efee55ef7..981c6e9504 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -97,7 +97,7 @@ fi
 export DEVELOPER=1
 export DEFAULT_TEST_TARGET=prove
 export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
-export GIT_TEST_OPTS="--verbose-log"
+export GIT_TEST_OPTS="--verbose-log --immediate"
 export GIT_TEST_CLONE_2GB=YesPlease
 
 case "$jobname" in
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 4f261ddc01..cc6a1247a4 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -23,5 +23,25 @@ do
echo "$(tput setaf 1)${TEST_OUT}...$(tput sgr0)"
echo 
""
cat "${TEST_OUT}"
+
+   TEST_NAME="${TEST_EXIT%.exit}"
+   TEST_NAME="${TEST_NAME##*/}"
+   TRASH_DIR="t/trash directory.$TEST_NAME"
+   TRASH_TGZ_B64="$TEST_NAME.trash.base64"
+   if [ -d "$TRASH_DIR" ]
+   then
+   tar czp "$TRASH_DIR" |base64 >"$TRASH_TGZ_B64"
+
+   if [ 1048576 -lt $(wc -c <"$TRASH_TGZ_B64") ]
+   then
+   # larger than 1MB
+   echo "$(tput setaf 1)Trash directory of 
'$TEST_NAME' is too big to be included in the trace log$(tput sgr0)"
+   else
+   echo "$(tput setaf 1)Trash directory of 
'$TEST_NAME':$(tput sgr0)"
+   echo "(Extract it to a file from the raw log, 
make sure it has Unix line endings, then run 'base64 -d  |tar vxzp' to 
restore it)"
+   cat "$TRASH_TGZ_B64"
+   echo "$(tput setaf 1)End of trash directory of 
'$TEST_NAME'$(tput sgr0)"
+   fi
+   fi
fi
 done
-- 
2.16.1.80.gc0eec9753d

Dearest

[Asana Hajraf]

Dearest,
I am Mrs. Asana Hajraf and I am married to Mr. Hassan Hajraf from kuwait for 19 
years without a child and my husband died in 2014. I am contacting you to let 
you know my desire to donate the sum of $4.5 million to charities in your 
country which I inherited from my late husband. Due to my illness of cancer 
were confirmed out of just eight months, so it is my desire to see that this 
money is invested to any organization of your choice and distributed each year 
among the charity organizations, motherless babies home, schools and support 
for the men and women homeless or what you may consider to be the benefit of 
those less fortunate. I do not want this fund to be invested in a manner 
without God. As soon as I receive your reply confirming your acceptance of the 
work I tell you, I will give all relevant information to authorize the release 
and transfer the money to you as my duly appointed representative.
Thanks,
Mrs Asana Hajraf.

Re: [PATCH] enable core.fsyncObjectFiles by default

[Theodore Ts'o]

On Mon, Jan 22, 2018 at 04:09:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > What's tricky is to devise a way to allow us to salvage objects that
> > are placed in a cruft pack because they are accessed recently,
> > proving themselves to be no longer crufts.  It could be that a good
> > way to resurrect them is to explode them to loose form when they are
> > accessed out of a cruft pack.  We need to worry about interactions
> > with read-only users if we go that route, but with the current
> > "explode unreachable to loose, touch their mtime when they are
> > accessed" scheme ends up ignoring accesses from read-only users that
> > cannot update mtime, so it might not be too bad.
> 
> Wouldn't it also make gc pruning more expensive? Now you can repack
> regularly and loose objects will be left out of the pack, and then just
> rm'd, whereas now it would entail creating new packs (unless the whole
> pack was objects meant for removal).

The idea is that the cruft pack would be all objects that were no
longer referenced.  Hence the proposal that if they ever *are*
accessed, they would be exploded to a loose object at that point.  So
in the common case, the GC would go quickly since the entire pack
could just be rm'ed once it hit the designated expiry time.

Another way of doing things would be to use the mtime of the cruft
pack for the expiry time, and if the curft pack is ever referenced,
its mtime would get updated.  Yet a third way would be to simply clear
the "cruft" bit if it ever *is* referenced.  In the common case, it
would never be referenced, so it could just get deleted, but in the
case where the user has manually "rescued" a set of commits (perhaps
by explicitly setting a branch head to commit id found from a reflog),
the objects would be saved.

So there are many ways it could be managed.

- Ted

Re: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit

[SZEDER Gábor]

On Wed, Jan 17, 2018 at 10:34 AM, Duy Nguyen  wrote:
> Actually I forgot another option. What if we automate updating the
> script at "compile" time instead of calling git at run time? E.g. with
> something like below, a contributor could just run
>
> make update-completion
>
> then add git-completion.bash changes to the same patch that introduces
> new options. If they forget

They inevitably will :)
If contributors have to remember something anyway, then they might
as well remember to update the completion script in the first place.

Another alternative would be to extend t9902 with (preferably
auto-generated) tests to compare the output of 'git $cmd
--git-completion-helper' with 'run_completion "git $cmd --"'.  Then
contributors wouldn't have to remember anything, because everyone runs
the full test suite every time anyway, right?

However, that would result in some code churn initially, because I
suspect the options are listed in different order in the command and
in the completion script.

All in all I don't think it would trump getting all --options straight
from the commands themselves.

SQUASH convert: add tracing for 'working-tree-encoding' attribute

[lars . schneider]

From: Lars Schneider 

Hi Junio,

this attached patch addresses Simon's review comments.

Can you squash the patch if you apply "[PATCH v4 5/6] convert: add
'working-tree-encoding' attribute"?

https://public-inbox.org/git/20180120152418.52859-6-lars.schnei...@autodesk.com/

Thanks,
Lars


 convert.c| 5 +++--
 t/t0028-working-tree-encoding.sh | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/convert.c b/convert.c
index 13fad490ce..f5e0cfc352 100644
--- a/convert.c
+++ b/convert.c
@@ -1165,8 +1165,9 @@ static struct encoding *git_path_check_encoding(struct 
attr_check_item *check)
if (!strcasecmp(value, default_encoding))
return NULL;

-   enc = xcalloc(1, sizeof(struct convert_driver));
-   enc->name = xstrdup_toupper(value);  /* aways use upper case names! */
+   enc = xcalloc(1, sizeof(*enc));
+   /* Aways use upper case names to simplify subsequent string comparison. 
*/
+   enc->name = xstrdup_toupper(value);
*encoding_tail = enc;
encoding_tail = &(enc->next);

diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh
index 0f36d4990a..a6da61280d 100755
--- a/t/t0028-working-tree-encoding.sh
+++ b/t/t0028-working-tree-encoding.sh
@@ -22,6 +22,8 @@ test_expect_success 'setup test repo' '
 test_expect_success 'ensure UTF-8 is stored in Git' '
git cat-file -p :test.utf16 >test.utf16.git &&
test_cmp_bin test.utf8.raw test.utf16.git &&
+
+   # cleanup
rm test.utf8.raw test.utf16.git
 '


base-commit: 21f4dac5aba07a6109285c57a0478bf502e09009
--
2.16.0

Re: git merge-tree: bug report and some feature requests

[Elijah Newren]

On Sat, Jan 20, 2018 at 7:00 PM, Josh Bleecher Snyder
 wrote:
> Hi, all.
>
> I'm experimenting with some new porcelain for interactive rebase. One
> goal is to leave the work tree untouched for most operations. It looks
> to me like 'git merge-tree' may be the right plumbing command for
> doing the merge part of the pick work of the todo list, one commit at
> a time. If I'm wrong about this, I'd love pointers; what follows may
> still be interesting anyway.

I don't have a concrete alternative (yet?) but here are some pointers
to two alternate merge-without-touching-working-tree possibilities, if
your current route doesn't pan out as well as you like:

I posted some patches last year to make merge-recursive.c be able to
do merges without touching the working tree.  Adding a few flags would
then enable it for any of 'merge', 'cherry-pick', 'am', or
'rebase'...though for unsuccessful merges, there's a clear question of
what/how conflicts should be reported to the user.  That probably
depends a fair amount on the precise use-case.

Although that series was placed on the backburner due to the immediate
driver of the feature going away, I'm still interested in such a
change, though I think it would fall out as a nice side effect of
implementing Junio's proposed ideal-world-merge-recursive rewrite[1].
I have started looking into that[2], but no guarantees about how
quickly I'll find time to finish or even whether I will.

[1] https://public-inbox.org/git/xmqqd147kpdm@gitster.mtv.corp.google.com
[2] https://github.com/newren/git/blob/ort/ort-cover-letter contains
overview of ideas and notes to myself about what I was hoping to
accomplish; currently it doesn't even compile or do anything

> 4. API suggestion
>
> Here's what I really want 'git merge-tree' to output. :)
...
> If the merge had conflicts, write the "as merged as possible" tree to

You'd need to define "as merged as possible" more carefully, because I
thought you meant a tree containing all the three-way merge conflict
markers and such being present in the "resolved" file, but from your
parenthetical note below it appears you think that is a different tree
that would also be useful to diff against the first one.  That leaves
me wondering what the first tree is. (Is it just the tree where for
each path, if that path had no conflicts associated with it then it's
the merge-resolved-file, and otherwise it's the file contents from the
merge-base?).

Both of these trees are actually rather non-trivial to define.  The
wording above isn't actually sufficient, because content conflicts
aren't the only kind of conflict.  More on that below.

There is already a bunch of code in merge-recursive.c to create a
forcibly-merged-accepting-conflict-markers-in-the-resolution and
record it as a tree (this is used for creating virtual merge bases in
the recursive case, namely when there isn't a single merge-base for
the two branches you are merging).  It might be reusable for what you
want here, but it's not immediately clear whether all the things it
does are appropriate; someone would have to consider the non-content
(path-based) conflicts carefully.

> the object database and give me its sha, and then also give me the
> three-way merge diff output for all conflicts, as a regular patch
> against that tree, using full path names and shas. (Alternatively,
> maybe better, give me a second sha for a tree containing all the
> three-way merge diff patches applied, which I can diff against the
> first tree to find the conflict patches.)

As far as I can tell, you're assuming that it's possible with two
trees that are crafted "just right", that you can tell where the merge
conflicts are, with binary files being your only difficulty.  Content
conflicts aren't the only type that exist; there are also path-based
conflicts.  These type of conflicts also make it difficult to know how
the two trees you are requesting should even be created.

For example, if there is a modify/delete conflict, how can that be
determined from just two trees?  If the first tree has the base
version of the file, then the second tree either has a file at the
same position or it doesn't.  Neither case looks like a conflict, but
the original merge had one.  You need more information.  The exact
same thing can be said for rename/delete conflicts.

Similarly, rename/add (one side renames an existing file to some new
path (say, "new_path"), and the other adds a brand new file at
"new_path), or rename/rename(2to1) (each side renames a different file
to the same location), won't be detectable just by diffing two trees.
These are often handled by moving both files to some other location,
so there's no way to record in a tree that there was a conflict.

rename/rename(1to2) is similar, but instead of two different original
files being renamed to the same thing, this is one file being renamed
differently on different sides of history.

I know that several of the examples above involved rename 

[PATCH] Use MOVE_ARRAY

[SZEDER Gábor]

Use the helper macro MOVE_ARRAY to move arrays.  This is shorter and
safer, as it automatically infers the size of elements.

Patch generated by Coccinelle and contrib/coccinelle/array.cocci in
Travis CI's static analysis build job.

Signed-off-by: SZEDER Gábor 
---
 cache-tree.c  | 5 ++---
 commit.c  | 6 ++
 diffcore-rename.c | 8 
 dir.c | 4 ++--
 parse-options.c   | 2 +-
 read-cache.c  | 5 ++---
 refs/ref-cache.c  | 6 ++
 replace_object.c  | 6 ++
 rerere.c  | 4 ++--
 9 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index e03e72c34a..18946aa458 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -84,9 +84,8 @@ static struct cache_tree_sub *find_subtree(struct cache_tree 
*it,
down->namelen = pathlen;
 
if (pos < it->subtree_nr)
-   memmove(it->down + pos + 1,
-   it->down + pos,
-   sizeof(down) * (it->subtree_nr - pos - 1));
+   MOVE_ARRAY(it->down + pos + 1, it->down + pos,
+  it->subtree_nr - pos - 1);
it->down[pos] = down;
return down;
 }
diff --git a/commit.c b/commit.c
index cab8d4455b..7d0080180d 100644
--- a/commit.c
+++ b/commit.c
@@ -126,10 +126,8 @@ int register_commit_graft(struct commit_graft *graft, int 
ignore_dups)
ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);
commit_graft_nr++;
if (pos < commit_graft_nr)
-   memmove(commit_graft + pos + 1,
-   commit_graft + pos,
-   (commit_graft_nr - pos - 1) *
-   sizeof(*commit_graft));
+   MOVE_ARRAY(commit_graft + pos + 1, commit_graft + pos,
+  commit_graft_nr - pos - 1);
commit_graft[pos] = graft;
return 0;
 }
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 245e999fe5..888a4b0189 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -57,8 +57,8 @@ static int add_rename_dst(struct diff_filespec *two)
ALLOC_GROW(rename_dst, rename_dst_nr + 1, rename_dst_alloc);
rename_dst_nr++;
if (first < rename_dst_nr)
-   memmove(rename_dst + first + 1, rename_dst + first,
-   (rename_dst_nr - first - 1) * sizeof(*rename_dst));
+   MOVE_ARRAY(rename_dst + first + 1, rename_dst + first,
+  rename_dst_nr - first - 1);
rename_dst[first].two = alloc_filespec(two->path);
fill_filespec(rename_dst[first].two, >oid, two->oid_valid,
  two->mode);
@@ -98,8 +98,8 @@ static struct diff_rename_src *register_rename_src(struct 
diff_filepair *p)
ALLOC_GROW(rename_src, rename_src_nr + 1, rename_src_alloc);
rename_src_nr++;
if (first < rename_src_nr)
-   memmove(rename_src + first + 1, rename_src + first,
-   (rename_src_nr - first - 1) * sizeof(*rename_src));
+   MOVE_ARRAY(rename_src + first + 1, rename_src + first,
+  rename_src_nr - first - 1);
rename_src[first].p = p;
rename_src[first].score = score;
return &(rename_src[first]);
diff --git a/dir.c b/dir.c
index 7c4b45e30e..ce6e50d2a2 100644
--- a/dir.c
+++ b/dir.c
@@ -747,8 +747,8 @@ static struct untracked_cache_dir *lookup_untracked(struct 
untracked_cache *uc,
FLEX_ALLOC_MEM(d, name, name, len);
 
ALLOC_GROW(dir->dirs, dir->dirs_nr + 1, dir->dirs_alloc);
-   memmove(dir->dirs + first + 1, dir->dirs + first,
-   (dir->dirs_nr - first) * sizeof(*dir->dirs));
+   MOVE_ARRAY(dir->dirs + first + 1, dir->dirs + first,
+  dir->dirs_nr - first);
dir->dirs_nr++;
dir->dirs[first] = d;
return d;
diff --git a/parse-options.c b/parse-options.c
index fca7159646..d02eb8b015 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -525,7 +525,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
 {
-   memmove(ctx->out + ctx->cpidx, ctx->argv, ctx->argc * 
sizeof(*ctx->out));
+   MOVE_ARRAY(ctx->out + ctx->cpidx, ctx->argv, ctx->argc);
ctx->out[ctx->cpidx + ctx->argc] = NULL;
return ctx->cpidx + ctx->argc;
 }
diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..2e8c85c686 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1217,9 +1217,8 @@ int add_index_entry(struct index_state *istate, struct 
cache_entry *ce, int opti
/* Add it in.. */
istate->cache_nr++;
if (istate->cache_nr > pos + 1)
-   memmove(istate->cache + pos + 1,
-   istate->cache + pos,
-   (istate->cache_nr - pos - 1) * sizeof(ce));
+   MOVE_ARRAY(istate->cache + pos + 1, istate->cache + pos,
+  istate->cache_nr - pos - 1);
set_index_entry(istate, 

Attention Email Id,Idd

[Mr.Larry mark]

Attention Email Id,

We have deposited the check of your fund ($5.500`000`00USD) through Western 
Union department after our finally meeting regarding your fund, All you will do 
is to 

contact Western Union director Pastor Fred Martins,(mrlarymarkm...@gmail.com)
He will give you direction on how you will be receiving the funds daily. 
Remember to send him your full Details,

Full information to avoid wrong transfer such as,

Receiver's Name:..
Address: ...
Country: ...
Phone Number: ..
Competition

Though, Agent: Mrs. Mary Collins has sent $5000 in your name today so contact 
Pastor Fred Martins or you call him +229-606616765 as soon as you receive this 
email and 

tell him to give you the Mtcn, sender name and question/answer to pick the 
$5000 Please let us know as soon as you
received all your fund,

Best Regards.
Mr.Larry mark

[ANNOUNCE] Git for Windows 2.16.1

[Johannes Schindelin]

Dear Git users,

It is my pleasure to announce that Git for Windows 2.16.1 is available from:

https://git-for-windows.github.io/

Changes since Git for Windows v2.16.0(2) (January 18th 2018)

This is a hotfix release, based on upstream Git's hotfix to address a
possible segmentation fault associated with case-insensitive file
systems.

Note: another hotfix might be coming the day after tomorrow, as cURL
announced a new version addressing security advisories that might
affect how Git talks via HTTP/HTTPS, too.

New Features

  * Comes with Git v2.16.1.

Bug Fixes

  * A set of regressions introduced by patches intended to speed up
reset and checkout was fixed (issues #1437, #1438, #1440 and #1442.

Filename | SHA-256
 | ---
Git-2.16.1-64-bit.exe | 
45ef5c5bd4b89c7d1a0126ecb37cd49f8d7f176458c1084149315bfc3af0eccd
Git-2.16.1-32-bit.exe | 
559886de7e5e4edcc3e4c5b1cb766ab90b37d4b7564a61770abdfd3cc84d53fc
PortableGit-2.16.1-64-bit.7z.exe | 
657296d33adceb1d7dabb17847dbd9ee5a45c4ef9d8d8aade2bfb42e57682baf
PortableGit-2.16.1-32-bit.7z.exe | 
bc8cf57a206ca63d92aad649322bcec6600a41b91be61229d9282f61e42834ed
MinGit-2.16.1-64-bit.zip | 
14fde7abfa14f505605517b00c8b1bf09eec78e3653516f30dc43084d8df7ede
MinGit-2.16.1-32-bit.zip | 
9a37a9dd89add49caee6967e7f6d54182b2f27de7bd4e63e3bd006394ca56e69
MinGit-2.16.1-busybox-64-bit.zip | 
21ac64a8876f96af8375b4ab655e669be99c341c1fe5c762485d720886f31b48
MinGit-2.16.1-busybox-32-bit.zip | 
158eac07dbd90555afcfb73709a33feffa070db5ae3ff6602bf400d26f326d37
Git-2.16.1-64-bit.tar.bz2 | 
7c81ae1be97362a39cafe9377cbb1c681343c24a23c1fe1e3c20fb77d98de557
Git-2.16.1-32-bit.tar.bz2 | 
654e88c5a07aa553353cbefd1f1adacff1d8c3caec7ecfbedf3ff7d8bbea708b

Ciao,
Johannes

Dear Friend, Your Urgent Respond Is Needed,

[Sherifar Abell]

Dear Friend

Please carefully read this Message and get back to me, if you can
handle this deal, My name is Mrs. Sherifar Abell. I am a banker
working with one of the well known Bank here in Burkina Faso as an
Auditing Manager, the name of the bank will be mention to you in
details when i receive you respond for security reasons.

During our last two years Auditing i come across an unclaimed Fund of
Nineteen million Three Hundred Thousand U.S Dollars ($19.300.000.00)
and after going through the Fund file i discovered that this Fund was
deposited by our late customer who died in a Car accident along side
with his family, he deposited the Fund as a contract Fund living no
one as a Beneficiary/ Next Of Kin to claim the Fund because the Money
was made for an already Awarded and sign contract before his Death.and
ever since then i have kept the Fund file to myself and after our
Auditing this past year i found out that no one made mention of  the
Fund file.

All this wile i have been doing under ground investigation concerning
the Fund and i found out that the Company of our late customer Mr ALI
Moisi SAAD, from SAUD ARABIA has been Fold as his staffs was unable to
manage the Company after his death.

Now i want to present you to our Bank as the Beneficiary of the Fund
using the information which i will later give you to apply as a
business partner to our Late customer, taking over to Execute the
already sign contract so that the Bank will transfer the Fund into
your Bank Account in your country and it shall be for the benefit of
both of us, as i promise to back you up with all the necessary
information concerning the Fund, There will be No Risk or problem in
this transaction since i have the profile of the Fund in my position.
Get back to me for more details if you can handle this transaction. I
will detail you with my information and photo to know more about me
upon you return.

Waiting for your urgent respond.

Sincerely,
Mrs.sherifar Abell

Re: [PATCH] enable core.fsyncObjectFiles by default

[Ævar Arnfjörð Bjarmason]

On Sat, Jan 20 2018, Junio C. Hamano jotted:

> Theodore Ts'o  writes:
>
>>   I've never been fond of the "git repack -A" behavior
>> where it can generate huge numbers of loose files.  I'd much prefer it
>> if the other objects ended up in a separate pack file, and then some
>> other provision made for nuking that pack file some time later
>
> Yes, a "cruft pack" that holds unreachable object has been discussed
> a few times recently on list, and I do agree that it is a desirable
> thing to have in the longer run.
>
> What's tricky is to devise a way to allow us to salvage objects that
> are placed in a cruft pack because they are accessed recently,
> proving themselves to be no longer crufts.  It could be that a good
> way to resurrect them is to explode them to loose form when they are
> accessed out of a cruft pack.  We need to worry about interactions
> with read-only users if we go that route, but with the current
> "explode unreachable to loose, touch their mtime when they are
> accessed" scheme ends up ignoring accesses from read-only users that
> cannot update mtime, so it might not be too bad.

Wouldn't it also make gc pruning more expensive? Now you can repack
regularly and loose objects will be left out of the pack, and then just
rm'd, whereas now it would entail creating new packs (unless the whole
pack was objects meant for removal).

Probably still worth it, but something to keep in mind.

[PATCH 1/5] travis-ci: use 'set -x' for the commands under 'su' in the 32 bit Linux build

[SZEDER Gábor]

Signed-off-by: SZEDER Gábor 
---
 ci/run-linux32-build.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index c19c50c1c9..5a36a8d7c0 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -26,6 +26,7 @@ test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID 
$CI_USER) &&
 
 # Build and test
 linux32 --32bit i386 su -m -l $CI_USER -c '
+set -x &&
 cd /usr/src/git &&
 ln -s /tmp/travis-cache/.prove t/.prove &&
 make --jobs=2 &&
-- 
2.16.1.80.gc0eec9753d

[PATCH 4/5] travis-ci: don't run the test suite as root in the 32 bit Linux build

[SZEDER Gábor]

Travis CI runs the 32 bit Linux build job in a Docker container, where
all commands are executed as root by default.  Therefore, ever since
we added this build job in 88dedd5e7 (Travis: also test on 32-bit
Linux, 2017-03-05), we have a bit of code to create a user in the
container matching the ID of the host user and then to run the test
suite as this user.  Matching the host user ID is important, because
otherwise the host user would have no access to any files written by
processes running in the container, notably the logs of failed tests
couldn't be included in the build job's trace log.

Alas, this piece of code never worked, because it sets the variable
holding the user name ($CI_USER) in a subshell, meaning it doesn't
have any effect by the time we get to the point to actually use the
variable to switch users with 'su'.  So all this time we were running
the test suite as root.

Reorganize that piece of code in 'ci/run-linux32-build.sh' a bit to
avoid that problematic subshell and to ensure that we switch to the
right user.  Furthermore, make the script's optional host user ID
option mandatory, so running the build accidentally as root will
become harder when debugging locally.  If someone really wants to run
the test suite as root, whatever the reasons might be, it'll still be
possible to do so by explicitly passing '0' as host user ID.

Finally, one last catch: since commit 7e72cfcee (travis-ci: save prove
state for the 32 bit Linux build, 2017-12-27) the 'prove' test harness
has been writing its state to the Travis CI cache directory from
within the Docker container while running as root.  After this patch
'prove' will run as a regular user, so in future build jobs it won't
be able overwrite a previously written, still root-owned state file,
resulting in build job failures.  To resolve this we should manually
delete caches containing such root-owned files, but that would be a
hassle.  Instead, work this around by changing the owner of the whole
contents of the cache directory to the host user ID.

Signed-off-by: SZEDER Gábor 
---
 ci/run-linux32-build.sh  | 30 +-
 ci/run-linux32-docker.sh |  2 +-
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index c9476d6598..e37e1d2d5f 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -3,11 +3,17 @@
 # Build and test Git in a 32-bit environment
 #
 # Usage:
-#   run-linux32-build.sh [host-user-id]
+#   run-linux32-build.sh 
 #
 
 set -ex
 
+if test $# -ne 1 || test -z "$1"
+then
+   echo >&2 "usage: run-linux32-build.sh "
+   exit 1
+fi
+
 # Update packages to the latest available versions
 linux32 --32bit i386 sh -c '
 apt update >/dev/null &&
@@ -18,11 +24,25 @@ linux32 --32bit i386 sh -c '
 # If this script runs inside a docker container, then all commands are
 # usually executed as root. Consequently, the host user might not be
 # able to access the test output files.
-# If a host user id is given, then create a user "ci" with the host user
-# id to make everything accessible to the host user.
+# If a non 0 host user id is given, then create a user "ci" with that
+# user id to make everything accessible to the host user.
 HOST_UID=$1
-CI_USER=$USER
-test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER)
+if test $HOST_UID -eq 0
+then
+   # Just in case someone does want to run the test suite as root.
+   CI_USER=root
+else
+   CI_USER=ci
+   useradd -u $HOST_UID $CI_USER
+   # Due to a bug the test suite was run as root in the past, so
+   # a prove state file created back then is only accessible by
+   # root.  Now that bug is fixed, the test suite is run as a
+   # regular user, but the prove state file coming from Travis
+   # CI's cache might still be owned by root.
+   # Make sure that this user has rights to any cached files,
+   # including an existing prove state file.
+   test -n "$cache_dir" && chown -R $HOST_UID:$HOST_UID "$cache_dir"
+fi
 
 # Build and test
 linux32 --32bit i386 su -m -l $CI_USER -c "
diff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh
index 15288ea2cf..21637903ce 100755
--- a/ci/run-linux32-docker.sh
+++ b/ci/run-linux32-docker.sh
@@ -9,7 +9,7 @@ docker pull daald/ubuntu32:xenial
 
 # Use the following command to debug the docker build locally:
 # $ docker run -itv "${PWD}:/usr/src/git" --entrypoint /bin/bash 
daald/ubuntu32:xenial
-# root@container:/# /usr/src/git/ci/run-linux32-build.sh
+# root@container:/# /usr/src/git/ci/run-linux32-build.sh 
 
 container_cache_dir=/tmp/travis-cache
 
-- 
2.16.1.80.gc0eec9753d

[PATCH 0/5] Travis CI: don't run the test suite as root in the 32 bit Linux build

[SZEDER Gábor]

The important one is patch 4, which fixes the issue of running the test
suite as root, with a bit of sugar on top to make sure that a future
"non-root" build job can cope with a root-written cache directory from
the past.

The rest is a collection of small cleanups and improvements to make the
debugging this Docked-based build more convenient.

SZEDER Gábor (5):
  travis-ci: use 'set -x' for the commands under 'su' in the 32 bit
Linux build
  travis-ci: use 'set -e' in the 32 bit Linux build job
  travis-ci: don't repeat the path of the cache directory
  travis-ci: don't run the test suite as root in the 32 bit Linux build
  travis-ci: don't fail if user already exists on 32 bit Linux build job

 ci/lib-travisci.sh|  7 +++---
 ci/run-build-and-tests.sh |  2 +-
 ci/run-linux32-build.sh   | 55 +++
 ci/run-linux32-docker.sh  |  7 --
 4 files changed, 51 insertions(+), 20 deletions(-)

-- 
2.16.1.80.gc0eec9753d

[PATCH 3/5] travis-ci: don't repeat the path of the cache directory

[SZEDER Gábor]

Some of our 'ci/*' scripts repeat the name or full path of the Travis
CI cache directory, and the following patches will add new places
using that path.

Use a variable to refer to the path of the cache directory instead, so
it's hard-coded only in a single place.

Pay extra attention to the 32 bit Linux build: it runs in a Docker
container, so pass the path of the cache directory from the host to
the container in an environment variable.  Furthermore, use the
variable in the container only if it's set, to prevent errors when
someone is running or debugging the Docker build locally, because in
that case the variable won't be set as there won't be any Travis CI
cache.

Signed-off-by: SZEDER Gábor 
---
 ci/lib-travisci.sh| 7 ---
 ci/run-build-and-tests.sh | 2 +-
 ci/run-linux32-build.sh   | 6 +++---
 ci/run-linux32-docker.sh  | 5 -
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 07f27c7270..1efee55ef7 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -21,8 +21,6 @@ skip_branch_tip_with_tag () {
fi
 }
 
-good_trees_file="$HOME/travis-cache/good-trees"
-
 # Save some info about the current commit's tree, so we can skip the build
 # job if we encounter the same tree again and can provide a useful info
 # message.
@@ -83,7 +81,10 @@ check_unignored_build_artifacts ()
 # and installing dependencies.
 set -ex
 
-mkdir -p "$HOME/travis-cache"
+cache_dir="$HOME/travis-cache"
+good_trees_file="$cache_dir/good-trees"
+
+mkdir -p "$cache_dir"
 
 skip_branch_tip_with_tag
 skip_good_tree
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index d3a094603f..30790e258d 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -5,7 +5,7 @@
 
 . ${0%/*}/lib-travisci.sh
 
-ln -s $HOME/travis-cache/.prove t/.prove
+ln -s "$cache_dir/.prove" t/.prove
 
 make --jobs=2
 make --quiet test
diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index 248183982b..c9476d6598 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -25,10 +25,10 @@ CI_USER=$USER
 test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER)
 
 # Build and test
-linux32 --32bit i386 su -m -l $CI_USER -c '
+linux32 --32bit i386 su -m -l $CI_USER -c "
set -ex
cd /usr/src/git
-   ln -s /tmp/travis-cache/.prove t/.prove
+   test -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove
make --jobs=2
make --quiet test
-'
+"
diff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh
index 4f191c5bb1..15288ea2cf 100755
--- a/ci/run-linux32-docker.sh
+++ b/ci/run-linux32-docker.sh
@@ -11,6 +11,8 @@ docker pull daald/ubuntu32:xenial
 # $ docker run -itv "${PWD}:/usr/src/git" --entrypoint /bin/bash 
daald/ubuntu32:xenial
 # root@container:/# /usr/src/git/ci/run-linux32-build.sh
 
+container_cache_dir=/tmp/travis-cache
+
 docker run \
--interactive \
--env DEVELOPER \
@@ -18,8 +20,9 @@ docker run \
--env GIT_PROVE_OPTS \
--env GIT_TEST_OPTS \
--env GIT_TEST_CLONE_2GB \
+   --env cache_dir="$container_cache_dir" \
--volume "${PWD}:/usr/src/git" \
-   --volume "${HOME}/travis-cache:/tmp/travis-cache" \
+   --volume "$cache_dir:$container_cache_dir" \
daald/ubuntu32:xenial \
/usr/src/git/ci/run-linux32-build.sh $(id -u $USER)
 
-- 
2.16.1.80.gc0eec9753d

[PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job

[SZEDER Gábor]

All 'ci/*' scripts use 'set -e' to break the build job if a command
fails, except 'ci/run-linux32-build.sh' which relies on the && chain
to do the same.  This inconsistency among the 'ci/*' scripts is asking
for trouble: I forgot about the && chain more than once while working
on this patch series.

Enable 'set -e' for the whole script and for the commands executed
under 'su' as well.

While touching every line in the 'su' command block anyway, change
their indentation to use a tab instead of spaces.

Signed-off-by: SZEDER Gábor 
---
 ci/run-linux32-build.sh | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index 5a36a8d7c0..248183982b 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -6,29 +6,29 @@
 #   run-linux32-build.sh [host-user-id]
 #
 
-set -x
+set -ex
 
 # Update packages to the latest available versions
 linux32 --32bit i386 sh -c '
 apt update >/dev/null &&
 apt install -y build-essential libcurl4-openssl-dev libssl-dev \
libexpat-dev gettext python >/dev/null
-' &&
+'
 
 # If this script runs inside a docker container, then all commands are
 # usually executed as root. Consequently, the host user might not be
 # able to access the test output files.
 # If a host user id is given, then create a user "ci" with the host user
 # id to make everything accessible to the host user.
-HOST_UID=$1 &&
-CI_USER=$USER &&
-test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) &&
+HOST_UID=$1
+CI_USER=$USER
+test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER)
 
 # Build and test
 linux32 --32bit i386 su -m -l $CI_USER -c '
-set -x &&
-cd /usr/src/git &&
-ln -s /tmp/travis-cache/.prove t/.prove &&
-make --jobs=2 &&
-make --quiet test
+   set -ex
+   cd /usr/src/git
+   ln -s /tmp/travis-cache/.prove t/.prove
+   make --jobs=2
+   make --quiet test
 '
-- 
2.16.1.80.gc0eec9753d

[PATCH 5/5] travis-ci: don't fail if user already exists on 32 bit Linux build job

[SZEDER Gábor]

The 32 bit Linux build job runs in a Docker container, which lends
itself to running and debugging locally, too.  Especially during
debugging one usually doesn't want to start with a fresh container
every time, to save time spent on installing a bunch of dependencies.
However, that doesn't work quite smootly, because the script running
in the container always creates a new user, which then must be removed
every time before subsequent executions, or the build script fails.

Make this process more convenient and don't try to create that user if
it already exists and has the right user ID in the container, so
developers don't have to bother with running a 'userdel' each time
before they run the build script.

The build job on Travis CI always starts with a fresh Docker
container, so this change doesn't make a difference there.

Signed-off-by: SZEDER Gábor 
---
 ci/run-linux32-build.sh | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index e37e1d2d5f..13047adde3 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -33,7 +33,13 @@ then
CI_USER=root
 else
CI_USER=ci
-   useradd -u $HOST_UID $CI_USER
+   if test "$(id -u $CI_USER)" = $HOST_UID
+   then
+   : # user already exists with the right ID
+   else
+   useradd -u $HOST_UID $CI_USER
+   fi
+
# Due to a bug the test suite was run as root in the past, so
# a prove state file created back then is only accessible by
# root.  Now that bug is fixed, the test suite is run as a
-- 
2.16.1.80.gc0eec9753d

Re: [PATCH v2 00/14] Some fixes and bunch of object_id conversions

[Patryk Obara]

> Patches 1 and 2 should be sent separately though.

I hoped they could be piggy-backed, but since there will be v3 of this patch
series... I'll wait for Junio's opinion.

> I look forward to seeing you deal with the object reading part …

Yeah, even reading of loose objects does not work on my test branch yet.

Turns out some functions (*) depend on each other during conversion to oid -
so they are either refactored together in one BIG commit or I need to deploy
some temporary hacks to make it possible to split work into smaller batches.

(*) refactor of sha1_object_info transitively depends on lookup_replace_object
and the other way around. verify_object and verify_tag will also cause some
problems.

-- 
| ← Ceci n'est pas une pipe
Patryk Obara

Re: [PATCH v2 07/14] match-trees: convert splice_tree to object_id

[Patryk Obara]

On 22 January 2018 at 12:56, Duy Nguyen  wrote:
> On Mon, Jan 22, 2018 at 12:04:30PM +0100, Patryk Obara wrote:
>> Convert the definition of static recursive splice_tree function to use
>> struct object_id and adjust single caller.
>>
>> Signed-off-by: Patryk Obara 
>> ---
>>  match-trees.c | 42 --
>>  1 file changed, 20 insertions(+), 22 deletions(-)
>>
>> diff --git a/match-trees.c b/match-trees.c
>> index 396b7338df..0f899a7212 100644
>> --- a/match-trees.c
>> +++ b/match-trees.c
>> @@ -161,19 +161,17 @@ static void match_trees(const struct object_id *hash1,
>>   * A tree "hash1" has a subdirectory at "prefix".  Come up with a
>>   * tree object by replacing it with another tree "hash2".
>>   */
>> -static int splice_tree(const unsigned char *hash1,
>> -const char *prefix,
>> -const unsigned char *hash2,
>> -unsigned char *result)
>> +static int splice_tree(const struct object_id *hash1, const char *prefix,
>> +const struct object_id *hash2, struct object_id *result)
>
> Maybe change the names to oid1 and oid2 too. I had a "what?" moment
> when I read hash1->hash below.

OK

>> @@ -197,26 +195,26 @@ static int splice_tree(const unsigned char *hash1,
>>   if (strlen(name) == toplen &&
>>   !memcmp(name, prefix, toplen)) {
>>   if (!S_ISDIR(mode))
>> - die("entry %s in tree %s is not a tree",
>> - name, sha1_to_hex(hash1));
>> - rewrite_here = (unsigned char *) oid->hash;
>> + die("entry %s in tree %s is not a tree", name,
>> + oid_to_hex(hash1));
>> + rewrite_here = (struct object_id *)oid;
>
> You don't need the typecast here anymore, do you?

Unfortunately, I do :(

Few lines above:
192: const struct object_id *oid;
194: oid = tree_entry_extract(, , );

Function tree_entry_extract returns const pointer, which leads to
compiler warning:
"assigning to 'struct object_id *' from 'const struct object_id *'
discards qualifiers".

On the other hand, if I change const qualifier for 'rewrite_here'
variable - warning will
appear in line 216:

216: oidcpy(rewrite_here, rewrite_with);

So the question here is rather: is it ok to overwrite buffer returned
by tree_entry_extract?

When writing this I opted to preserve cv-qualifiers despite changing
pointer type (which
implied preservation of typecast) - partly because parameter 'desc' of
tree_entry_extract
is NOT const (which suggests to me, that it's ok).

But this cast might be indication of unintended modification inside
tree description
structure and might lead to an error is some other place, if there's
an assumption, that
this buffer is not overwritable.

Maybe const should be removed from return type of tree_entry_extract (and maybe
from oid field of struct name_entry)?

I will give it some more thought - maybe oidcpy from line 216 could be replaced.

-- 
| ← Ceci n'est pas une pipe
Patryk Obara

Re: [PATCH v2 05/14] sha1_file: convert hash_sha1_file to object_id

[Duy Nguyen]

On Mon, Jan 22, 2018 at 7:44 PM, Patryk Obara  wrote:
> On 22 January 2018 at 12:49, Duy Nguyen  wrote:
>> On Mon, Jan 22, 2018 at 12:04:28PM +0100, Patryk Obara wrote:
>>> @@ -969,7 +969,7 @@ static int ident_to_worktree(const char *path, const 
>>> char *src, size_t len,
>>>
>>>   /* step 4: substitute */
>>>   strbuf_addstr(buf, "Id: ");
>>> - strbuf_add(buf, sha1_to_hex(sha1), 40);
>>> + strbuf_add(buf, sha1_to_hex(oid.hash), GIT_SHA1_HEXSZ);
>>
>> oid_to_hex()?
>
> I didn't do it originally because the size of hash is explicitly
> passed as the third parameter.

Aha! I didn't see that.

> I should probably replace this line with:
>
> strbuf_addstr(buf, oid_to_hex());
>
> ... since a hex representation is correctly 0-delimited anyway.

Yeah I think that's a good idea. This 40 came from 3fed15f568 (Add
'ident' conversion. - 2007-04-21). Back then memcpy was used and 40
made sense. The conversion to strbuf should have used strbuf_addstr()
as you suggested, unless there's performance concerns, but I don't
think there is any.
-- 
Duy

Re: [PATCH v2 05/14] sha1_file: convert hash_sha1_file to object_id

[Patryk Obara]

On 22 January 2018 at 12:49, Duy Nguyen  wrote:
> On Mon, Jan 22, 2018 at 12:04:28PM +0100, Patryk Obara wrote:
>> @@ -969,7 +969,7 @@ static int ident_to_worktree(const char *path, const 
>> char *src, size_t len,
>>
>>   /* step 4: substitute */
>>   strbuf_addstr(buf, "Id: ");
>> - strbuf_add(buf, sha1_to_hex(sha1), 40);
>> + strbuf_add(buf, sha1_to_hex(oid.hash), GIT_SHA1_HEXSZ);
>
> oid_to_hex()?

I didn't do it originally because the size of hash is explicitly
passed as the third parameter.
I should probably replace this line with:

strbuf_addstr(buf, oid_to_hex());

... since a hex representation is correctly 0-delimited anyway.
Will include in v3 unless there'll be some other suggestion :)

-- 
| ← Ceci n'est pas une pipe
Patryk Obara

Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute

[Lars Schneider]

> On 21 Jan 2018, at 15:22, Simon Ruderich  wrote:
> 
> On Sat, Jan 20, 2018 at 04:24:17PM +0100, lars.schnei...@autodesk.com wrote:
>> +static struct encoding *git_path_check_encoding(struct attr_check_item 
>> *check)
>> +{
>> +const char *value = check->value;
>> +struct encoding *enc;
>> +
>> +if (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value) ||
>> +!strlen(value))
>> +return NULL;
>> +
>> +for (enc = encoding; enc; enc = enc->next)
>> +if (!strcasecmp(value, enc->name))
>> +return enc;
>> +
>> +/* Don't encode to the default encoding */
>> +if (!strcasecmp(value, default_encoding))
>> +return NULL;
>> +
>> +enc = xcalloc(1, sizeof(struct convert_driver));
> 
> I think this should be "sizeof(struct encoding)" but I prefer
> "sizeof(*enc)" which prevents these kind of mistakes.

Great catch! Thank you!

Other code paths are at risk of this problem too. Consider this:

$ git grep 'sizeof(\*' | wc -l
 303
$ git grep 'sizeof(struct ' | wc -l
 208

E.g. even in the same file (likely where I got the code from):
https://github.com/git/git/blob/59c276cf4da0705064c32c9dba54baefa282ea55/convert.c#L780

@Junio: 
Would you welcome a patch that replaces "struct foo" with "*foo"
if applicable?


>> +enc->name = xstrdup_toupper(value);  /* aways use upper case names! */
> 
> "aways" -> "always" and I think the comment should say why
> uppercase is important.

Would that be better?

/* Aways use upper case names to simplify subsequent string comparison. 
*/
enc->name = xstrdup_toupper(value);

AFAIK uppercase and lowercase names are both valid. I just wanted to
ensure that we use one consistent casing. That reads better in error messages
and I don't need to check for the letter case in has_prohibited_utf_bom()
and friends in utf8.c


>> +test_expect_success 'ensure UTF-8 is stored in Git' '
>> +git cat-file -p :test.utf16 >test.utf16.git &&
>> +test_cmp_bin test.utf8.raw test.utf16.git &&
>> +rm test.utf8.raw test.utf16.git
>> +'
>> +
>> +test_expect_success 're-encode to UTF-16 on checkout' '
>> +rm test.utf16 &&
>> +git checkout test.utf16 &&
>> +test_cmp_bin test.utf16.raw test.utf16 &&
>> +
>> +# cleanup
>> +rm test.utf16.raw
> 
> Micro-nit: For consistency with the previous test, remove the
> empty line and comment (or just keep the files generated from the
> "setup test repo" phase and don't explicitly delete them)?

I would rather add a new line and a comment to the previous test 
to be consistent.

I know we could leave the files but these lingering files could
always surprise writers of future tests (at least they surprised
me in other tests).


Thank you very much for the review,
Lars

[PATCH] format-patch: set diffstat width to 70 instead of default 80

[Nguyễn Thái Ngọc Duy]

Patches or cover letters generated by format-patch are meant to be
exchanged as emails, most of the time. And since it's generally agreed
that text in mails should be wrapped around 70 columns or so, make sure
these diffstat follow the convention.

I noticed this when I quoted a diffstat line [1]. Should we do something
like this? diffstat is rarely quoted though so perhaps the stat width
should be something like 75.

t4052 fails but I don't think it's worth fixing until it's clear if it's
worth doing this.

[1] https://public-inbox.org/git/20180122121426.GD5980@ash/T/#u

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/log.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 14fdf39165..6be79656c5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
 
memcpy(, >diffopt, sizeof(opts));
opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
+   opts.diffopt.stat_width = 70;
 
diff_setup_done();
 
@@ -1611,9 +1612,12 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
die(_("--check does not make sense"));
 
if (!use_patch_format &&
-   (!rev.diffopt.output_format ||
-rev.diffopt.output_format == DIFF_FORMAT_PATCH))
+   (!rev.diffopt.output_format ||
+rev.diffopt.output_format == DIFF_FORMAT_PATCH)) {
rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | 
DIFF_FORMAT_SUMMARY;
+   if (!rev.diffopt.stat_width)
+   rev.diffopt.stat_width = 70;
+   }
 
/* Always generate a patch */
rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
-- 
2.16.0.47.g3d9b0fac3a

Re: [PATCH v2 00/14] Some fixes and bunch of object_id conversions

[Duy Nguyen]

On Mon, Jan 22, 2018 at 12:04:23PM +0100, Patryk Obara wrote:
>  sha1_file.c   | 100 
> ++

You have started the invasion to sha1_file.c. Victory is near! (*)
2018 will be the year of something-other-shan-sha1 :)

I've read through the series quickly. It looks good. Patches 1 and 2
should be sent separately though.

(*) Technically the hashing part is still easy. I look forward to
seeing you deal with the object reading part, especially from pack
files. It's fun ;-)
--
Duy

Re: [PATCH v2 10/14] notes: convert write_notes_tree to object_id

[Duy Nguyen]

On Mon, Jan 22, 2018 at 12:04:33PM +0100, Patryk Obara wrote:
> @@ -1141,12 +1142,13 @@ int write_notes_tree(struct notes_tree *t, unsigned 
> char *result)
>   cb_data.next_non_note = t->first_non_note;
>  
>   /* Write tree objects representing current notes tree */
> - ret = for_each_note(t, FOR_EACH_NOTE_DONT_UNPACK_SUBTREES |
> - FOR_EACH_NOTE_YIELD_SUBTREES,
> - write_each_note, _data) ||
> - write_each_non_note_until(NULL, _data) ||
> - tree_write_stack_finish_subtree() ||
> - write_sha1_file(root.buf.buf, root.buf.len, tree_type, result);
> + flags = FOR_EACH_NOTE_DONT_UNPACK_SUBTREES |
> + FOR_EACH_NOTE_YIELD_SUBTREES;
> + ret = for_each_note(t, flags, write_each_note, _data) ||
> +   write_each_non_note_until(NULL, _data) ||
> +   tree_write_stack_finish_subtree() ||
> +   write_sha1_file(root.buf.buf, root.buf.len, tree_type,
> +   result->hash);

Hmm.. new indentation style. I'm not complaining though. I think it
looks good.

--
Duy

Re: [PATCH v2 07/14] match-trees: convert splice_tree to object_id

[Duy Nguyen]

On Mon, Jan 22, 2018 at 12:04:30PM +0100, Patryk Obara wrote:
> Convert the definition of static recursive splice_tree function to use
> struct object_id and adjust single caller.
> 
> Signed-off-by: Patryk Obara 
> ---
>  match-trees.c | 42 --
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/match-trees.c b/match-trees.c
> index 396b7338df..0f899a7212 100644
> --- a/match-trees.c
> +++ b/match-trees.c
> @@ -161,19 +161,17 @@ static void match_trees(const struct object_id *hash1,
>   * A tree "hash1" has a subdirectory at "prefix".  Come up with a
>   * tree object by replacing it with another tree "hash2".
>   */
> -static int splice_tree(const unsigned char *hash1,
> -const char *prefix,
> -const unsigned char *hash2,
> -unsigned char *result)
> +static int splice_tree(const struct object_id *hash1, const char *prefix,
> +const struct object_id *hash2, struct object_id *result)

Maybe change the names to oid1 and oid2 too. I had a "what?" moment
when I read hash1->hash below.

> @@ -197,26 +195,26 @@ static int splice_tree(const unsigned char *hash1,
>   if (strlen(name) == toplen &&
>   !memcmp(name, prefix, toplen)) {
>   if (!S_ISDIR(mode))
> - die("entry %s in tree %s is not a tree",
> - name, sha1_to_hex(hash1));
> - rewrite_here = (unsigned char *) oid->hash;
> + die("entry %s in tree %s is not a tree", name,
> + oid_to_hex(hash1));
> + rewrite_here = (struct object_id *)oid;

You don't need the typecast here anymore, do you?

--
Duy

Re: [PATCH v2 05/14] sha1_file: convert hash_sha1_file to object_id

[Duy Nguyen]

On Mon, Jan 22, 2018 at 12:04:28PM +0100, Patryk Obara wrote:
> @@ -969,7 +969,7 @@ static int ident_to_worktree(const char *path, const char 
> *src, size_t len,
>  
>   /* step 4: substitute */
>   strbuf_addstr(buf, "Id: ");
> - strbuf_add(buf, sha1_to_hex(sha1), 40);
> + strbuf_add(buf, sha1_to_hex(oid.hash), GIT_SHA1_HEXSZ);

oid_to_hex()?

--
Duy

Re: [PATCH] worktree: teach "add" to check out existing branches

[Duy Nguyen]

On Sun, Jan 21, 2018 at 7:02 PM, Thomas Gummerer  wrote:
> Currently 'git worktree add ' creates a new branch named after the
> basename of the path by default.  If a branch with that name already
> exists, the command refuses to do anything, unless the '--force' option
> is given.
>
> However we can do a little better than that, and check the branch out if
> it is not checked out anywhere else.  This will help users who just want
> to check an existing branch out into a new worktree, and save a few
> keystrokes.
>
> As the current behaviour is to simply 'die()' when a brach with the name
> of the basename of the path already exists, there are no backwards
> compatibility worries here.
>
> We will still 'die()' if the branch is checked out in another worktree,
> unless the --force flag is passed.
>
> Signed-off-by: Thomas Gummerer 
> ---
>
> This is a follow-up to
> https://public-inbox.org/git/20171118181345.GC32324@hank/, where this
> was first suggested, but I didn't want to do it as part of that
> series.  Now I finally got around to implementing it.
>
>  Documentation/git-worktree.txt |  9 +++--
>  builtin/worktree.c | 22 +++---
>  t/t2025-worktree-add.sh| 15 ---
>  3 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 41585f535d..98731b71a7 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -61,8 +61,13 @@ $ git worktree add --track -b   
> /
>  
>  +
>  If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
> -then, as a convenience, a new branch based at HEAD is created automatically,
> -as if `-b $(basename )` was specified.
> +then, as a convenience, a worktree with a branch named after
> +`$(basename )` (call it ``) is created.  If ``
> +doesn't exist, a new branch based on HEAD is automatically created as
> +if `-b ` was given.  If `` exists in the repository,
> +it will be checked out in the new worktree, if it's not checked out
> +anywhere else, otherwise the command will refuse to create the
> +worktree.

It starts getting a bit too magical to me. We probably should print
something "switching to branch..." or "creating new branch ..."  to
let people know what decision was taken, unless --quiet is given. Yeah
I know --quiet does not exist. You don't need to add it now either
since nobody has asked for it.

More or less related to this, there was a question [1] whether we
could do better than $(basename ) for determining branch name.
Since you're doing start to check if a branch exists, people may start
asking to check for branch "foo/bar" from the path abc/foo/bar instead
of just branch "bar".

[1] https://github.com/git-for-windows/git/issues/1390

>
>  list::
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7cef5b120b..148a864bb9 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -411,13 +411,21 @@ static int add(int ac, const char **av, const char 
> *prefix)
> if (ac < 2 && !opts.new_branch && !opts.detach) {
> int n;
> const char *s = worktree_basename(path, );
> -   opts.new_branch = xstrndup(s, n);
> -   if (guess_remote) {
> -   struct object_id oid;
> -   const char *remote =
> -   unique_tracking_name(opts.new_branch, );
> -   if (remote)
> -   branch = remote;
> +   const char *branchname = xstrndup(s, n);
> +   struct strbuf ref = STRBUF_INIT;

Perhaps a blank line after this to separate var declarations and the rest.

> +   if (!strbuf_check_branch_ref(, branchname) &&
> +   ref_exists(ref.buf)) {
> +   branch = branchname;

Hmm.. do we need UNLEAK(branch);? Maybe you should try valgrind, I'm not sure.

> +   opts.checkout = 1;
> +   } else {
> +   opts.new_branch = branchname;
> +   if (guess_remote) {
> +   struct object_id oid;
> +   const char *remote =
> +   unique_tracking_name(opts.new_branch, 
> );
> +   if (remote)
> +   branch = remote;
> +   }
> }
> }
>
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 2b95944973..721b0e4c26 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -198,13 +198,22 @@ test_expect_success '"add" with  omitted' '
> test_cmp_rev HEAD bat
>  '
>
> -test_expect_success '"add" auto-vivify does not clobber existing branch' '
> +test_expect_success '"add" auto-vivify checks out existing branch' '
> 

[PATCH v2 09/14] notes: convert combine_notes_* to object_id

[Patryk Obara]

Convert the definition and declarations of combine_notes_* functions
to struct object_id and adjust usage of these functions.

Signed-off-by: Patryk Obara 
---
 notes.c | 46 +++---
 notes.h | 25 +++--
 2 files changed, 38 insertions(+), 33 deletions(-)

diff --git a/notes.c b/notes.c
index c7f21fae44..3f4f94507a 100644
--- a/notes.c
+++ b/notes.c
@@ -270,8 +270,8 @@ static int note_tree_insert(struct notes_tree *t, struct 
int_node *tree,
if (!oidcmp(>val_oid, >val_oid))
return 0;
 
-   ret = combine_notes(l->val_oid.hash,
-   entry->val_oid.hash);
+   ret = combine_notes(>val_oid,
+   >val_oid);
if (!ret && is_null_oid(>val_oid))
note_tree_remove(t, tree, n, entry);
free(entry);
@@ -786,8 +786,8 @@ static int prune_notes_helper(const struct object_id 
*object_oid,
return 0;
 }
 
-int combine_notes_concatenate(unsigned char *cur_sha1,
-   const unsigned char *new_sha1)
+int combine_notes_concatenate(struct object_id *cur_oid,
+ const struct object_id *new_oid)
 {
char *cur_msg = NULL, *new_msg = NULL, *buf;
unsigned long cur_len, new_len, buf_len;
@@ -795,18 +795,18 @@ int combine_notes_concatenate(unsigned char *cur_sha1,
int ret;
 
/* read in both note blob objects */
-   if (!is_null_sha1(new_sha1))
-   new_msg = read_sha1_file(new_sha1, _type, _len);
+   if (!is_null_oid(new_oid))
+   new_msg = read_sha1_file(new_oid->hash, _type, _len);
if (!new_msg || !new_len || new_type != OBJ_BLOB) {
free(new_msg);
return 0;
}
-   if (!is_null_sha1(cur_sha1))
-   cur_msg = read_sha1_file(cur_sha1, _type, _len);
+   if (!is_null_oid(cur_oid))
+   cur_msg = read_sha1_file(cur_oid->hash, _type, _len);
if (!cur_msg || !cur_len || cur_type != OBJ_BLOB) {
free(cur_msg);
free(new_msg);
-   hashcpy(cur_sha1, new_sha1);
+   oidcpy(cur_oid, new_oid);
return 0;
}
 
@@ -825,20 +825,20 @@ int combine_notes_concatenate(unsigned char *cur_sha1,
free(new_msg);
 
/* create a new blob object from buf */
-   ret = write_sha1_file(buf, buf_len, blob_type, cur_sha1);
+   ret = write_sha1_file(buf, buf_len, blob_type, cur_oid->hash);
free(buf);
return ret;
 }
 
-int combine_notes_overwrite(unsigned char *cur_sha1,
-   const unsigned char *new_sha1)
+int combine_notes_overwrite(struct object_id *cur_oid,
+   const struct object_id *new_oid)
 {
-   hashcpy(cur_sha1, new_sha1);
+   oidcpy(cur_oid, new_oid);
return 0;
 }
 
-int combine_notes_ignore(unsigned char *cur_sha1,
-   const unsigned char *new_sha1)
+int combine_notes_ignore(struct object_id *cur_oid,
+const struct object_id *new_oid)
 {
return 0;
 }
@@ -848,17 +848,17 @@ int combine_notes_ignore(unsigned char *cur_sha1,
  * newlines removed.
  */
 static int string_list_add_note_lines(struct string_list *list,
- const unsigned char *sha1)
+ const struct object_id *oid)
 {
char *data;
unsigned long len;
enum object_type t;
 
-   if (is_null_sha1(sha1))
+   if (is_null_oid(oid))
return 0;
 
/* read_sha1_file NUL-terminates */
-   data = read_sha1_file(sha1, , );
+   data = read_sha1_file(oid->hash, , );
if (t != OBJ_BLOB || !data || !len) {
free(data);
return t != OBJ_BLOB || !data;
@@ -884,17 +884,17 @@ static int string_list_join_lines_helper(struct 
string_list_item *item,
return 0;
 }
 
-int combine_notes_cat_sort_uniq(unsigned char *cur_sha1,
-   const unsigned char *new_sha1)
+int combine_notes_cat_sort_uniq(struct object_id *cur_oid,
+   const struct object_id *new_oid)
 {
struct string_list sort_uniq_list = STRING_LIST_INIT_DUP;
struct strbuf buf = STRBUF_INIT;
int ret = 1;
 
/* read both note blob objects into unique_lines */
-   if (string_list_add_note_lines(_uniq_list, cur_sha1))
+   if (string_list_add_note_lines(_uniq_list, cur_oid))
goto out;
-   if (string_list_add_note_lines(_uniq_list, new_sha1))
+   if (string_list_add_note_lines(_uniq_list, new_oid))
goto out;
string_list_remove_empty_items(_uniq_list, 0);
string_list_sort(_uniq_list);

[PATCH v2 14/14] sha1_file: rename hash_sha1_file_literally

[Patryk Obara]

This function was already converted to use struct object_id earlier.
---
 builtin/hash-object.c | 3 ++-
 cache.h   | 4 +++-
 sha1_file.c   | 5 +++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index c532ff9320..526da5c185 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -24,7 +24,8 @@ static int hash_literally(struct object_id *oid, int fd, 
const char *type, unsig
if (strbuf_read(, fd, 4096) < 0)
ret = -1;
else
-   ret = hash_sha1_file_literally(buf.buf, buf.len, type, oid, 
flags);
+   ret = hash_object_file_literally(buf.buf, buf.len, type, oid,
+flags);
strbuf_release();
return ret;
 }
diff --git a/cache.h b/cache.h
index 0a8be9c87f..6ef4248931 100644
--- a/cache.h
+++ b/cache.h
@@ -1243,7 +1243,9 @@ extern int hash_object_file(const void *buf, unsigned 
long len,
 extern int write_object_file(const void *buf, unsigned long len,
 const char *type, struct object_id *oid);
 
-extern int hash_sha1_file_literally(const void *buf, unsigned long len, const 
char *type, struct object_id *oid, unsigned flags);
+extern int hash_object_file_literally(const void *buf, unsigned long len,
+ const char *type, struct object_id *oid,
+ unsigned flags);
 
 extern int pretend_object_file(void *, unsigned long, enum object_type,
   struct object_id *oid);
diff --git a/sha1_file.c b/sha1_file.c
index 59238f5bea..34c041e8cd 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1652,8 +1652,9 @@ int write_object_file(const void *buf, unsigned long len, 
const char *type,
return write_loose_object(oid, hdr, hdrlen, buf, len, 0);
 }
 
-int hash_sha1_file_literally(const void *buf, unsigned long len, const char 
*type,
-struct object_id *oid, unsigned flags)
+int hash_object_file_literally(const void *buf, unsigned long len,
+  const char *type, struct object_id *oid,
+  unsigned flags)
 {
char *header;
int hdrlen, status = 0;
-- 
2.14.3

[PATCH v2 11/14] sha1_file: convert write_sha1_file to object_id

[Patryk Obara]

Convert the definition and declaration of write_sha1_file to
struct object_id and adjust usage of this function.

This commit also converts static function write_sha1_file_prepare, as it
is closely related.

Rename these functions to write_object_file and
write_object_file_prepare respectively.

Replace sha1_to_hex, hashcpy and hashclr with their oid equivalents
wherever possible.

Signed-off-by: Patryk Obara 
---
 apply.c  |  8 
 builtin/checkout.c   |  3 +--
 builtin/mktag.c  |  6 +++---
 builtin/mktree.c | 10 +-
 builtin/notes.c  |  8 
 builtin/receive-pack.c   | 11 ++-
 builtin/replace.c|  2 +-
 builtin/tag.c|  2 +-
 builtin/unpack-objects.c |  9 ++---
 cache-tree.c |  5 +++--
 cache.h  |  4 +++-
 commit.c |  2 +-
 match-trees.c|  2 +-
 merge-recursive.c|  5 +++--
 notes-cache.c|  2 +-
 notes.c  |  9 -
 read-cache.c |  6 +++---
 sha1_file.c  | 29 +++--
 18 files changed, 65 insertions(+), 58 deletions(-)

diff --git a/apply.c b/apply.c
index 57ab8a8a29..4cd4504008 100644
--- a/apply.c
+++ b/apply.c
@@ -3554,7 +3554,7 @@ static int try_threeway(struct apply_state *state,
 
/* Preimage the patch was prepared for */
if (patch->is_new)
-   write_sha1_file("", 0, blob_type, pre_oid.hash);
+   write_object_file("", 0, blob_type, _oid);
else if (get_oid(patch->old_sha1_prefix, _oid) ||
 read_blob_object(, _oid, patch->old_mode))
return error(_("repository lacks the necessary blob to fall 
back on 3-way merge."));
@@ -3570,7 +3570,7 @@ static int try_threeway(struct apply_state *state,
return -1;
}
/* post_oid is theirs */
-   write_sha1_file(tmp_image.buf, tmp_image.len, blob_type, post_oid.hash);
+   write_object_file(tmp_image.buf, tmp_image.len, blob_type, _oid);
clear_image(_image);
 
/* our_oid is ours */
@@ -3583,7 +3583,7 @@ static int try_threeway(struct apply_state *state,
return error(_("cannot read the current contents of 
'%s'"),
 patch->old_name);
}
-   write_sha1_file(tmp_image.buf, tmp_image.len, blob_type, our_oid.hash);
+   write_object_file(tmp_image.buf, tmp_image.len, blob_type, _oid);
clear_image(_image);
 
/* in-core three-way merge between post and our using pre as base */
@@ -4291,7 +4291,7 @@ static int add_index_file(struct apply_state *state,
}
fill_stat_cache_info(ce, );
}
-   if (write_sha1_file(buf, size, blob_type, ce->oid.hash) < 0) {
+   if (write_object_file(buf, size, blob_type, >oid) < 0) {
free(ce);
return error(_("unable to create backing store "
   "for newly created file %s"), path);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8bdc927d3f..ebb6b44660 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -227,8 +227,7 @@ static int checkout_merged(int pos, const struct checkout 
*state)
 * (it also writes the merge result to the object database even
 * when it may contain conflicts).
 */
-   if (write_sha1_file(result_buf.ptr, result_buf.size,
-   blob_type, oid.hash))
+   if (write_object_file(result_buf.ptr, result_buf.size, blob_type, ))
die(_("Unable to add merge result for '%s'"), path);
free(result_buf.ptr);
ce = make_cache_entry(mode, oid.hash, path, 2, 0);
diff --git a/builtin/mktag.c b/builtin/mktag.c
index 031b750f06..beb552847b 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -151,7 +151,7 @@ static int verify_tag(char *buffer, unsigned long size)
 int cmd_mktag(int argc, const char **argv, const char *prefix)
 {
struct strbuf buf = STRBUF_INIT;
-   unsigned char result_sha1[20];
+   struct object_id result;
 
if (argc != 1)
usage("git mktag");
@@ -165,10 +165,10 @@ int cmd_mktag(int argc, const char **argv, const char 
*prefix)
if (verify_tag(buf.buf, buf.len) < 0)
die("invalid tag signature file");
 
-   if (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)
+   if (write_object_file(buf.buf, buf.len, tag_type, ) < 0)
die("unable to write tag file");
 
strbuf_release();
-   printf("%s\n", sha1_to_hex(result_sha1));
+   printf("%s\n", oid_to_hex());
return 0;
 }
diff --git a/builtin/mktree.c b/builtin/mktree.c
index da0fd8cd70..8dd9f52f77 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -40,7 +40,7 @@ static int ent_compare(const void *a_, const void *b_)
  

[PATCH v2 07/14] match-trees: convert splice_tree to object_id

[Patryk Obara]

Convert the definition of static recursive splice_tree function to use
struct object_id and adjust single caller.

Signed-off-by: Patryk Obara 
---
 match-trees.c | 42 --
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/match-trees.c b/match-trees.c
index 396b7338df..0f899a7212 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -161,19 +161,17 @@ static void match_trees(const struct object_id *hash1,
  * A tree "hash1" has a subdirectory at "prefix".  Come up with a
  * tree object by replacing it with another tree "hash2".
  */
-static int splice_tree(const unsigned char *hash1,
-  const char *prefix,
-  const unsigned char *hash2,
-  unsigned char *result)
+static int splice_tree(const struct object_id *hash1, const char *prefix,
+  const struct object_id *hash2, struct object_id *result)
 {
char *subpath;
int toplen;
char *buf;
unsigned long sz;
struct tree_desc desc;
-   unsigned char *rewrite_here;
-   const unsigned char *rewrite_with;
-   unsigned char subtree[20];
+   struct object_id *rewrite_here;
+   const struct object_id *rewrite_with;
+   struct object_id subtree;
enum object_type type;
int status;
 
@@ -182,9 +180,9 @@ static int splice_tree(const unsigned char *hash1,
if (*subpath)
subpath++;
 
-   buf = read_sha1_file(hash1, , );
+   buf = read_sha1_file(hash1->hash, , );
if (!buf)
-   die("cannot read tree %s", sha1_to_hex(hash1));
+   die("cannot read tree %s", oid_to_hex(hash1));
init_tree_desc(, buf, sz);
 
rewrite_here = NULL;
@@ -197,26 +195,26 @@ static int splice_tree(const unsigned char *hash1,
if (strlen(name) == toplen &&
!memcmp(name, prefix, toplen)) {
if (!S_ISDIR(mode))
-   die("entry %s in tree %s is not a tree",
-   name, sha1_to_hex(hash1));
-   rewrite_here = (unsigned char *) oid->hash;
+   die("entry %s in tree %s is not a tree", name,
+   oid_to_hex(hash1));
+   rewrite_here = (struct object_id *)oid;
break;
}
update_tree_entry();
}
if (!rewrite_here)
-   die("entry %.*s not found in tree %s",
-   toplen, prefix, sha1_to_hex(hash1));
+   die("entry %.*s not found in tree %s", toplen, prefix,
+   oid_to_hex(hash1));
if (*subpath) {
-   status = splice_tree(rewrite_here, subpath, hash2, subtree);
+   status = splice_tree(rewrite_here, subpath, hash2, );
if (status)
return status;
-   rewrite_with = subtree;
-   }
-   else
+   rewrite_with = 
+   } else {
rewrite_with = hash2;
-   hashcpy(rewrite_here, rewrite_with);
-   status = write_sha1_file(buf, sz, tree_type, result);
+   }
+   oidcpy(rewrite_here, rewrite_with);
+   status = write_sha1_file(buf, sz, tree_type, result->hash);
free(buf);
return status;
 }
@@ -280,7 +278,7 @@ void shift_tree(const struct object_id *hash1,
if (!*add_prefix)
return;
 
-   splice_tree(hash1->hash, add_prefix, hash2->hash, shifted->hash);
+   splice_tree(hash1, add_prefix, hash2, shifted);
 }
 
 /*
@@ -334,7 +332,7 @@ void shift_tree_by(const struct object_id *hash1,
 * shift tree2 down by adding shift_prefix above it
 * to match tree1.
 */
-   splice_tree(hash1->hash, shift_prefix, hash2->hash, 
shifted->hash);
+   splice_tree(hash1, shift_prefix, hash2, shifted);
else
/*
 * shift tree2 up by removing shift_prefix from it
-- 
2.14.3

[PATCH v2 03/14] sha1_file: convert pretend_sha1_file to object_id

[Patryk Obara]

Convert the declaration and definition of pretend_sha1_file to use
struct object_id and adjust all usages of this function.  Rename it to
pretend_object_file.

Signed-off-by: Patryk Obara 
---
 Documentation/technical/api-object-access.txt |  2 +-
 blame.c   |  2 +-
 cache.h   |  5 -
 sha1_file.c   | 10 +-
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/technical/api-object-access.txt 
b/Documentation/technical/api-object-access.txt
index 03bb0e950d..a1162e5bcd 100644
--- a/Documentation/technical/api-object-access.txt
+++ b/Documentation/technical/api-object-access.txt
@@ -7,7 +7,7 @@ Talk about  and  family, things like
 * read_object_with_reference()
 * has_sha1_file()
 * write_sha1_file()
-* pretend_sha1_file()
+* pretend_object_file()
 * lookup_{object,commit,tag,blob,tree}
 * parse_{object,commit,tag,blob,tree}
 * Use of object flags
diff --git a/blame.c b/blame.c
index 2893f3c103..1fc22b304b 100644
--- a/blame.c
+++ b/blame.c
@@ -232,7 +232,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
convert_to_git(_index, path, buf.buf, buf.len, , 0);
origin->file.ptr = buf.buf;
origin->file.size = buf.len;
-   pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_oid.hash);
+   pretend_object_file(buf.buf, buf.len, OBJ_BLOB, >blob_oid);
 
/*
 * Read the current index, replace the path entry with
diff --git a/cache.h b/cache.h
index d8b975a571..e4e03ac51d 100644
--- a/cache.h
+++ b/cache.h
@@ -1241,7 +1241,10 @@ extern int sha1_object_info(const unsigned char *, 
unsigned long *);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char 
*type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char 
*type, unsigned char *return_sha1);
 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const 
char *type, struct object_id *oid, unsigned flags);
-extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned 
char *);
+
+extern int pretend_object_file(void *, unsigned long, enum object_type,
+  struct object_id *oid);
+
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern int git_open_cloexec(const char *name, int flags);
 #define git_open(name) git_open_cloexec(name, O_RDONLY)
diff --git a/sha1_file.c b/sha1_file.c
index 3da70ac650..830b93b428 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1312,13 +1312,13 @@ static void *read_object(const unsigned char *sha1, 
enum object_type *type,
return content;
 }
 
-int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
- unsigned char *sha1)
+int pretend_object_file(void *buf, unsigned long len, enum object_type type,
+   struct object_id *oid)
 {
struct cached_object *co;
 
-   hash_sha1_file(buf, len, typename(type), sha1);
-   if (has_sha1_file(sha1) || find_cached_object(sha1))
+   hash_sha1_file(buf, len, typename(type), oid->hash);
+   if (has_sha1_file(oid->hash) || find_cached_object(oid->hash))
return 0;
ALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc);
co = _objects[cached_object_nr++];
@@ -1326,7 +1326,7 @@ int pretend_sha1_file(void *buf, unsigned long len, enum 
object_type type,
co->type = type;
co->buf = xmalloc(len);
memcpy(co->buf, buf, len);
-   hashcpy(co->sha1, sha1);
+   hashcpy(co->sha1, oid->hash);
return 0;
 }
 
-- 
2.14.3

[PATCH v2 00/14] Some fixes and bunch of object_id conversions

[Patryk Obara]

Compared to v1:

Following brian's suggestion I renamed following functions and struct
names to indicate, that they are no longer intended for sha1 algorithm
only:

struct sha1_stat -> struct oid_stat
pretend_sha1_file-> pretend_object_file
write_sha1_file  -> write_object_file
hash_sha1_file   -> hash_object_file
hash_sha1_file_literally -> hash_object_file_literally

Added two more patches converting some more functions to struct object_id
and one with pure function rename.

Patryk Obara (14):
  http-push: improve error log
  clang-format: adjust penalty for return type line break
  sha1_file: convert pretend_sha1_file to object_id
  dir: convert struct sha1_stat to use object_id
  sha1_file: convert hash_sha1_file to object_id
  cache: clear whole hash buffer with oidclr
  match-trees: convert splice_tree to object_id
  commit: convert commit_tree* to object_id
  notes: convert combine_notes_* to object_id
  notes: convert write_notes_tree to object_id
  sha1_file: convert write_sha1_file to object_id
  sha1_file: convert force_object_loose to object_id
  sha1_file: convert write_loose_object to object_id
  sha1_file: rename hash_sha1_file_literally

 .clang-format |   2 +-
 Documentation/technical/api-object-access.txt |   2 +-
 apply.c   |  12 ++--
 blame.c   |   2 +-
 builtin/am.c  |   4 +-
 builtin/checkout.c|   3 +-
 builtin/commit-tree.c |   4 +-
 builtin/commit.c  |   5 +-
 builtin/hash-object.c |   3 +-
 builtin/index-pack.c  |   5 +-
 builtin/merge.c   |   8 +--
 builtin/mktag.c   |   6 +-
 builtin/mktree.c  |  10 +--
 builtin/notes.c   |   8 +--
 builtin/pack-objects.c|   2 +-
 builtin/receive-pack.c|  11 +--
 builtin/replace.c |   4 +-
 builtin/tag.c |   2 +-
 builtin/unpack-objects.c  |  11 +--
 cache-tree.c  |  16 ++---
 cache.h   |  25 ---
 commit.c  |  15 ++--
 commit.h  |  11 +--
 convert.c |   6 +-
 diffcore-rename.c |   4 +-
 dir.c |  56 +++
 dir.h |  12 ++--
 http-push.c   |   4 ++
 log-tree.c|   2 +-
 match-trees.c |  42 ++-
 merge-recursive.c |   5 +-
 notes-cache.c |   8 +--
 notes-merge.c |   9 ++-
 notes-utils.c |   9 +--
 notes-utils.h |   3 +-
 notes.c   |  63 
 notes.h   |  29 
 read-cache.c  |   6 +-
 sha1_file.c   | 100 ++
 t/helper/test-dump-untracked-cache.c  |   4 +-
 40 files changed, 279 insertions(+), 254 deletions(-)


base-commit: 59c276cf4da0705064c32c9dba54baefa282ea55
-- 
2.14.3

[PATCH v2 13/14] sha1_file: convert write_loose_object to object_id

[Patryk Obara]

Convert the definition and declaration of statis write_loose_object
function to struct object_id.

Signed-off-by: Patryk Obara 
---
 sha1_file.c | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index d9ee966d74..59238f5bea 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1548,16 +1548,17 @@ static int create_tmpfile(struct strbuf *tmp, const 
char *filename)
return fd;
 }
 
-static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
- const void *buf, unsigned long len, time_t mtime)
+static int write_loose_object(const struct object_id *oid, char *hdr,
+ int hdrlen, const void *buf, unsigned long len,
+ time_t mtime)
 {
int fd, ret;
unsigned char compressed[4096];
git_zstream stream;
git_SHA_CTX c;
-   unsigned char parano_sha1[20];
+   struct object_id parano_oid;
static struct strbuf tmp_file = STRBUF_INIT;
-   const char *filename = sha1_file_name(sha1);
+   const char *filename = sha1_file_name(oid->hash);
 
fd = create_tmpfile(_file, filename);
if (fd < 0) {
@@ -1594,13 +1595,16 @@ static int write_loose_object(const unsigned char 
*sha1, char *hdr, int hdrlen,
} while (ret == Z_OK);
 
if (ret != Z_STREAM_END)
-   die("unable to deflate new object %s (%d)", sha1_to_hex(sha1), 
ret);
+   die("unable to deflate new object %s (%d)", oid_to_hex(oid),
+   ret);
ret = git_deflate_end_gently();
if (ret != Z_OK)
-   die("deflateEnd on object %s failed (%d)", sha1_to_hex(sha1), 
ret);
-   git_SHA1_Final(parano_sha1, );
-   if (hashcmp(sha1, parano_sha1) != 0)
-   die("confused by unstable object source data for %s", 
sha1_to_hex(sha1));
+   die("deflateEnd on object %s failed (%d)", oid_to_hex(oid),
+   ret);
+   git_SHA1_Final(parano_oid.hash, );
+   if (oidcmp(oid, _oid) != 0)
+   die("confused by unstable object source data for %s",
+   oid_to_hex(oid));
 
close_sha1_file(fd);
 
@@ -1645,7 +1649,7 @@ int write_object_file(const void *buf, unsigned long len, 
const char *type,
write_object_file_prepare(buf, len, type, oid, hdr, );
if (freshen_packed_object(oid->hash) || freshen_loose_object(oid->hash))
return 0;
-   return write_loose_object(oid->hash, hdr, hdrlen, buf, len, 0);
+   return write_loose_object(oid, hdr, hdrlen, buf, len, 0);
 }
 
 int hash_sha1_file_literally(const void *buf, unsigned long len, const char 
*type,
@@ -1663,7 +1667,7 @@ int hash_sha1_file_literally(const void *buf, unsigned 
long len, const char *typ
goto cleanup;
if (freshen_packed_object(oid->hash) || freshen_loose_object(oid->hash))
goto cleanup;
-   status = write_loose_object(oid->hash, header, hdrlen, buf, len, 0);
+   status = write_loose_object(oid, header, hdrlen, buf, len, 0);
 
 cleanup:
free(header);
@@ -1685,7 +1689,7 @@ int force_object_loose(const struct object_id *oid, 
time_t mtime)
if (!buf)
return error("cannot read sha1_file for %s", oid_to_hex(oid));
hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(type), len) + 1;
-   ret = write_loose_object(oid->hash, hdr, hdrlen, buf, len, mtime);
+   ret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime);
free(buf);
 
return ret;
-- 
2.14.3

[PATCH v2 12/14] sha1_file: convert force_object_loose to object_id

[Patryk Obara]

Convert the definition and declaration of force_object_loose to
struct object_id and adjust usage of this function.

Signed-off-by: Patryk Obara 
---
 builtin/pack-objects.c |  2 +-
 cache.h|  3 ++-
 sha1_file.c| 10 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6b9cfc289d..f38197543d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2768,7 +2768,7 @@ static void loosen_unused_packed_objects(struct rev_info 
*revs)
if (!packlist_find(_pack, oid.hash, NULL) &&
!has_sha1_pack_kept_or_nonlocal() &&
!loosened_object_can_be_discarded(, p->mtime))
-   if (force_object_loose(oid.hash, p->mtime))
+   if (force_object_loose(, p->mtime))
die("unable to force loose object");
}
}
diff --git a/cache.h b/cache.h
index d80141eb64..0a8be9c87f 100644
--- a/cache.h
+++ b/cache.h
@@ -1248,7 +1248,8 @@ extern int hash_sha1_file_literally(const void *buf, 
unsigned long len, const ch
 extern int pretend_object_file(void *, unsigned long, enum object_type,
   struct object_id *oid);
 
-extern int force_object_loose(const unsigned char *sha1, time_t mtime);
+extern int force_object_loose(const struct object_id *oid, time_t mtime);
+
 extern int git_open_cloexec(const char *name, int flags);
 #define git_open(name) git_open_cloexec(name, O_RDONLY)
 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
diff --git a/sha1_file.c b/sha1_file.c
index d1569b1b96..d9ee966d74 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1670,7 +1670,7 @@ int hash_sha1_file_literally(const void *buf, unsigned 
long len, const char *typ
return status;
 }
 
-int force_object_loose(const unsigned char *sha1, time_t mtime)
+int force_object_loose(const struct object_id *oid, time_t mtime)
 {
void *buf;
unsigned long len;
@@ -1679,13 +1679,13 @@ int force_object_loose(const unsigned char *sha1, 
time_t mtime)
int hdrlen;
int ret;
 
-   if (has_loose_object(sha1))
+   if (has_loose_object(oid->hash))
return 0;
-   buf = read_object(sha1, , );
+   buf = read_object(oid->hash, , );
if (!buf)
-   return error("cannot read sha1_file for %s", sha1_to_hex(sha1));
+   return error("cannot read sha1_file for %s", oid_to_hex(oid));
hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %lu", typename(type), len) + 1;
-   ret = write_loose_object(sha1, hdr, hdrlen, buf, len, mtime);
+   ret = write_loose_object(oid->hash, hdr, hdrlen, buf, len, mtime);
free(buf);
 
return ret;
-- 
2.14.3

[PATCH v2 05/14] sha1_file: convert hash_sha1_file to object_id

[Patryk Obara]

Convert the declaration and definition of hash_sha1_file to use
struct object_id and adjust all function calls.

Rename this function to hash_object_file.

Signed-off-by: Patryk Obara 
---
 apply.c  |  4 ++--
 builtin/index-pack.c |  5 ++---
 builtin/replace.c|  2 +-
 builtin/unpack-objects.c |  2 +-
 cache-tree.c | 11 +--
 cache.h  |  5 -
 convert.c|  6 +++---
 diffcore-rename.c|  4 ++--
 dir.c|  4 ++--
 log-tree.c   |  2 +-
 sha1_file.c  | 26 +-
 11 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/apply.c b/apply.c
index 321a9fa68d..57ab8a8a29 100644
--- a/apply.c
+++ b/apply.c
@@ -3154,7 +3154,7 @@ static int apply_binary(struct apply_state *state,
 * See if the old one matches what the patch
 * applies to.
 */
-   hash_sha1_file(img->buf, img->len, blob_type, oid.hash);
+   hash_object_file(img->buf, img->len, blob_type, );
if (strcmp(oid_to_hex(), patch->old_sha1_prefix))
return error(_("the patch applies to '%s' (%s), "
   "which does not match the "
@@ -3199,7 +3199,7 @@ static int apply_binary(struct apply_state *state,
 name);
 
/* verify that the result matches */
-   hash_sha1_file(img->buf, img->len, blob_type, oid.hash);
+   hash_object_file(img->buf, img->len, blob_type, );
if (strcmp(oid_to_hex(), patch->new_sha1_prefix))
return error(_("binary patch to '%s' creates incorrect 
result (expecting %s, got %s)"),
name, patch->new_sha1_prefix, oid_to_hex());
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 4c51aec81f..7f5a95e6ff 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -958,9 +958,8 @@ static void resolve_delta(struct object_entry *delta_obj,
free(delta_data);
if (!result->data)
bad_object(delta_obj->idx.offset, _("failed to apply delta"));
-   hash_sha1_file(result->data, result->size,
-  typename(delta_obj->real_type),
-  delta_obj->idx.oid.hash);
+   hash_object_file(result->data, result->size,
+typename(delta_obj->real_type), _obj->idx.oid);
sha1_object(result->data, NULL, result->size, delta_obj->real_type,
_obj->idx.oid);
counter_lock();
diff --git a/builtin/replace.c b/builtin/replace.c
index 10078ae371..814bf6bfde 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -355,7 +355,7 @@ static void check_one_mergetag(struct commit *commit,
struct tag *tag;
int i;
 
-   hash_sha1_file(extra->value, extra->len, typename(OBJ_TAG), 
tag_oid.hash);
+   hash_object_file(extra->value, extra->len, typename(OBJ_TAG), _oid);
tag = lookup_tag(_oid);
if (!tag)
die(_("bad mergetag in commit '%s'"), ref);
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 62ea264c46..85a40d1af7 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -258,7 +258,7 @@ static void write_object(unsigned nr, enum object_type type,
} else {
struct object *obj;
int eaten;
-   hash_sha1_file(buf, size, typename(type), 
obj_list[nr].oid.hash);
+   hash_object_file(buf, size, typename(type), _list[nr].oid);
added_object(nr, type, buf, size);
obj = parse_object_buffer(_list[nr].oid, type, size, buf,
  );
diff --git a/cache-tree.c b/cache-tree.c
index e03e72c34a..6574eeb80d 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -400,15 +400,14 @@ static int update_one(struct cache_tree *it,
}
 
if (repair) {
-   unsigned char sha1[20];
-   hash_sha1_file(buffer.buf, buffer.len, tree_type, sha1);
-   if (has_sha1_file(sha1))
-   hashcpy(it->oid.hash, sha1);
+   struct object_id oid;
+   hash_object_file(buffer.buf, buffer.len, tree_type, );
+   if (has_sha1_file(oid.hash))
+   oidcpy(>oid, );
else
to_invalidate = 1;
} else if (dryrun)
-   hash_sha1_file(buffer.buf, buffer.len, tree_type,
-  it->oid.hash);
+   hash_object_file(buffer.buf, buffer.len, tree_type, >oid);
else if (write_sha1_file(buffer.buf, buffer.len, tree_type, 
it->oid.hash)) {
strbuf_release();
return -1;
diff --git a/cache.h b/cache.h
index ed72933ba7..08f2b81e1b 100644
--- a/cache.h
+++ b/cache.h
@@ -1236,7 +1236,10 @@ 

[PATCH v2 08/14] commit: convert commit_tree* to object_id

[Patryk Obara]

Convert the definitions and declarations of commit_tree and
commit_tree_extended to use struct object_id and adjust all usages of
these functions.

Signed-off-by: Patryk Obara 
---
 builtin/am.c  |  4 ++--
 builtin/commit-tree.c |  4 ++--
 builtin/commit.c  |  5 +++--
 builtin/merge.c   |  8 
 commit.c  | 15 +++
 commit.h  | 11 ++-
 notes-cache.c |  4 ++--
 notes-merge.c |  9 -
 notes-utils.c |  7 ---
 notes-utils.h |  3 ++-
 10 files changed, 36 insertions(+), 34 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index acfe9d3c8c..6e6abb05cd 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1641,8 +1641,8 @@ static void do_commit(const struct am_state *state)
setenv("GIT_COMMITTER_DATE",
state->ignore_date ? "" : state->author_date, 1);
 
-   if (commit_tree(state->msg, state->msg_len, tree.hash, parents, 
commit.hash,
-   author, state->sign_commit))
+   if (commit_tree(state->msg, state->msg_len, , parents, ,
+   author, state->sign_commit))
die(_("failed to write commit object"));
 
reflog_msg = getenv("GIT_REFLOG_ACTION");
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 2177251e24..e5bdf57b1e 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -117,8 +117,8 @@ int cmd_commit_tree(int argc, const char **argv, const char 
*prefix)
die_errno("git commit-tree: failed to read");
}
 
-   if (commit_tree(buffer.buf, buffer.len, tree_oid.hash, parents,
-   commit_oid.hash, NULL, sign_commit)) {
+   if (commit_tree(buffer.buf, buffer.len, _oid, parents, _oid,
+   NULL, sign_commit)) {
strbuf_release();
return 1;
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 8a87701414..c14878302e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1792,8 +1792,9 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
append_merge_tag_headers(parents, );
}
 
-   if (commit_tree_extended(sb.buf, sb.len, active_cache_tree->oid.hash,
-parents, oid.hash, author_ident.buf, sign_commit, 
extra)) {
+   if (commit_tree_extended(sb.buf, sb.len, _cache_tree->oid,
+parents, , author_ident.buf, sign_commit,
+extra)) {
rollback_index_files();
die(_("failed to write commit object"));
}
diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..92ba99a1a5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -820,8 +820,8 @@ static int merge_trivial(struct commit *head, struct 
commit_list *remoteheads)
pptr = commit_list_append(head, pptr);
pptr = commit_list_append(remoteheads->item, pptr);
prepare_to_commit(remoteheads);
-   if (commit_tree(merge_msg.buf, merge_msg.len, result_tree.hash, parents,
-   result_commit.hash, NULL, sign_commit))
+   if (commit_tree(merge_msg.buf, merge_msg.len, _tree, parents,
+   _commit, NULL, sign_commit))
die(_("failed to write commit object"));
finish(head, remoteheads, _commit, "In-index merge");
drop_save();
@@ -845,8 +845,8 @@ static int finish_automerge(struct commit *head,
commit_list_insert(head, );
strbuf_addch(_msg, '\n');
prepare_to_commit(remoteheads);
-   if (commit_tree(merge_msg.buf, merge_msg.len, result_tree->hash, 
parents,
-   result_commit.hash, NULL, sign_commit))
+   if (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,
+   _commit, NULL, sign_commit))
die(_("failed to write commit object"));
strbuf_addf(, "Merge made by the '%s' strategy.", wt_strategy);
finish(head, remoteheads, _commit, buf.buf);
diff --git a/commit.c b/commit.c
index cab8d4455b..760137e54b 100644
--- a/commit.c
+++ b/commit.c
@@ -1395,9 +1395,8 @@ void free_commit_extra_headers(struct commit_extra_header 
*extra)
}
 }
 
-int commit_tree(const char *msg, size_t msg_len,
-   const unsigned char *tree,
-   struct commit_list *parents, unsigned char *ret,
+int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree,
+   struct commit_list *parents, struct object_id *ret,
const char *author, const char *sign_commit)
 {
struct commit_extra_header *extra = NULL, **tail = 
@@ -1526,8 +1525,8 @@ N_("Warning: commit message did not conform to UTF-8.\n"
"variable i18n.commitencoding to the encoding your project uses.\n");
 
 int commit_tree_extended(const char *msg, size_t msg_len,
-const unsigned 

[PATCH v2 01/14] http-push: improve error log

[Patryk Obara]

When git push fails due to server-side WebDAV error, it's not easy to
point to the main culprit.  Additional information about exact cURL
error and HTTP server response is helpful for debugging purpose.

New error log helped me pinpoint failing test t5540-http-push-webdav
to a missing Apache dependency in Fedora 27:
https://bugzilla.redhat.com/show_bug.cgi?id=1491151

Signed-off-by: Patryk Obara 
---
 http-push.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/http-push.c b/http-push.c
index 14435ab65d..0913f8ab86 100644
--- a/http-push.c
+++ b/http-push.c
@@ -915,6 +915,10 @@ static struct remote_lock *lock_remote(const char *path, 
long timeout)
lock->timeout = -1;
}
XML_ParserFree(parser);
+   } else {
+   fprintf(stderr,
+   "error: curl result=%d, HTTP code=%ld\n",
+   results.curl_result, results.http_code);
}
} else {
fprintf(stderr, "Unable to start LOCK request\n");
-- 
2.14.3

[PATCH v2 10/14] notes: convert write_notes_tree to object_id

[Patryk Obara]

Convert the definition and declaration of write_notes_tree to
struct object_id and adjust usage of this function.

Additionally, improve style of small part of this function, as old
formatting made it hard to understand at glance what this part of
code is doing.

Signed-off-by: Patryk Obara 
---
 notes-cache.c |  2 +-
 notes-utils.c |  2 +-
 notes.c   | 16 +---
 notes.h   |  4 ++--
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/notes-cache.c b/notes-cache.c
index d2f87147cc..010ad236cb 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -54,7 +54,7 @@ int notes_cache_write(struct notes_cache *c)
if (!c->tree.dirty)
return 0;
 
-   if (write_notes_tree(>tree, tree_oid.hash))
+   if (write_notes_tree(>tree, _oid))
return -1;
if (commit_tree(c->validity, strlen(c->validity), _oid, NULL,
_oid, NULL, NULL) < 0)
diff --git a/notes-utils.c b/notes-utils.c
index 058c642dac..02407fe2a7 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -12,7 +12,7 @@ void create_notes_commit(struct notes_tree *t, struct 
commit_list *parents,
 
assert(t->initialized);
 
-   if (write_notes_tree(t, tree_oid.hash))
+   if (write_notes_tree(t, _oid))
die("Failed to write notes tree to database");
 
if (!parents) {
diff --git a/notes.c b/notes.c
index 3f4f94507a..09ef1ce33a 100644
--- a/notes.c
+++ b/notes.c
@@ -1123,11 +1123,12 @@ int for_each_note(struct notes_tree *t, int flags, 
each_note_fn fn,
return for_each_note_helper(t, t->root, 0, 0, flags, fn, cb_data);
 }
 
-int write_notes_tree(struct notes_tree *t, unsigned char *result)
+int write_notes_tree(struct notes_tree *t, struct object_id *result)
 {
struct tree_write_stack root;
struct write_each_note_data cb_data;
int ret;
+   int flags;
 
if (!t)
t = _notes_tree;
@@ -1141,12 +1142,13 @@ int write_notes_tree(struct notes_tree *t, unsigned 
char *result)
cb_data.next_non_note = t->first_non_note;
 
/* Write tree objects representing current notes tree */
-   ret = for_each_note(t, FOR_EACH_NOTE_DONT_UNPACK_SUBTREES |
-   FOR_EACH_NOTE_YIELD_SUBTREES,
-   write_each_note, _data) ||
-   write_each_non_note_until(NULL, _data) ||
-   tree_write_stack_finish_subtree() ||
-   write_sha1_file(root.buf.buf, root.buf.len, tree_type, result);
+   flags = FOR_EACH_NOTE_DONT_UNPACK_SUBTREES |
+   FOR_EACH_NOTE_YIELD_SUBTREES;
+   ret = for_each_note(t, flags, write_each_note, _data) ||
+ write_each_non_note_until(NULL, _data) ||
+ tree_write_stack_finish_subtree() ||
+ write_sha1_file(root.buf.buf, root.buf.len, tree_type,
+ result->hash);
strbuf_release();
return ret;
 }
diff --git a/notes.h b/notes.h
index 88da38b5f4..0433f45db5 100644
--- a/notes.h
+++ b/notes.h
@@ -217,7 +217,7 @@ int for_each_note(struct notes_tree *t, int flags, 
each_note_fn fn,
  * Write the given notes_tree structure to the object database
  *
  * Creates a new tree object encapsulating the current state of the given
- * notes_tree, and stores its SHA1 into the 'result' argument.
+ * notes_tree, and stores its object id into the 'result' argument.
  *
  * Returns zero on success, non-zero on failure.
  *
@@ -225,7 +225,7 @@ int for_each_note(struct notes_tree *t, int flags, 
each_note_fn fn,
  * this function has returned zero. Please also remember to create a
  * corresponding commit object, and update the appropriate notes ref.
  */
-int write_notes_tree(struct notes_tree *t, unsigned char *result);
+int write_notes_tree(struct notes_tree *t, struct object_id *result);
 
 /* Flags controlling the operation of prune */
 #define NOTES_PRUNE_VERBOSE 1
-- 
2.14.3

[PATCH v2 02/14] clang-format: adjust penalty for return type line break

[Patryk Obara]

The penalty of 5 makes clang-format very eager to put even short type
declarations (e.g. "extern int") into a separate line, even when
breaking parameters list is sufficient.

Signed-off-by: Patryk Obara 
---
 .clang-format | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.clang-format b/.clang-format
index 611ab4750b..12a89f95f9 100644
--- a/.clang-format
+++ b/.clang-format
@@ -163,7 +163,7 @@ PenaltyBreakComment: 10
 PenaltyBreakFirstLessLess: 0
 PenaltyBreakString: 10
 PenaltyExcessCharacter: 100
-PenaltyReturnTypeOnItsOwnLine: 5
+PenaltyReturnTypeOnItsOwnLine: 60
 
 # Don't sort #include's
 SortIncludes: false
-- 
2.14.3

[PATCH v2 04/14] dir: convert struct sha1_stat to use object_id

[Patryk Obara]

Convert the declaration of struct sha1_stat. Adjust all usages of this
struct and replace hash{clr,cmp,cpy} with oid{clr,cmp,cpy} wherever
possible.  Rename it to struct oid_stat.

Remove macro EMPTY_BLOB_SHA1_BIN, as it's no longer used.

Signed-off-by: Patryk Obara 
---
 cache.h  |  2 --
 dir.c| 56 +---
 dir.h| 12 
 t/helper/test-dump-untracked-cache.c |  4 +--
 4 files changed, 34 insertions(+), 40 deletions(-)

diff --git a/cache.h b/cache.h
index e4e03ac51d..ed72933ba7 100644
--- a/cache.h
+++ b/cache.h
@@ -1047,8 +1047,6 @@ extern const struct object_id empty_tree_oid;
"\xe6\x9d\xe2\x9b\xb2\xd1\xd6\x43\x4b\x8b" \
"\x29\xae\x77\x5a\xd8\xc2\xe4\x8c\x53\x91"
 extern const struct object_id empty_blob_oid;
-#define EMPTY_BLOB_SHA1_BIN (empty_blob_oid.hash)
-
 
 static inline int is_empty_blob_sha1(const unsigned char *sha1)
 {
diff --git a/dir.c b/dir.c
index 7c4b45e30e..ef977b8657 100644
--- a/dir.c
+++ b/dir.c
@@ -233,10 +233,8 @@ int within_depth(const char *name, int namelen,
  *
  * Optionally updates the given sha1_stat with the given OID (when valid).
  */
-static int do_read_blob(const struct object_id *oid,
-   struct sha1_stat *sha1_stat,
-   size_t *size_out,
-   char **data_out)
+static int do_read_blob(const struct object_id *oid, struct oid_stat 
*sha1_stat,
+   size_t *size_out, char **data_out)
 {
enum object_type type;
unsigned long sz;
@@ -253,7 +251,7 @@ static int do_read_blob(const struct object_id *oid,
 
if (sha1_stat) {
memset(_stat->stat, 0, sizeof(sha1_stat->stat));
-   hashcpy(sha1_stat->sha1, oid->hash);
+   oidcpy(_stat->oid, oid);
}
 
if (sz == 0) {
@@ -654,9 +652,8 @@ void add_exclude(const char *string, const char *base,
 
 static int read_skip_worktree_file_from_index(const struct index_state *istate,
  const char *path,
- size_t *size_out,
- char **data_out,
- struct sha1_stat *sha1_stat)
+ size_t *size_out, char **data_out,
+ struct oid_stat *sha1_stat)
 {
int pos, len;
 
@@ -795,9 +792,8 @@ static int add_excludes_from_buffer(char *buf, size_t size,
  * ss_valid is non-zero, "ss" must contain good value as input.
  */
 static int add_excludes(const char *fname, const char *base, int baselen,
-   struct exclude_list *el,
-   struct index_state *istate,
-   struct sha1_stat *sha1_stat)
+   struct exclude_list *el, struct index_state *istate,
+   struct oid_stat *sha1_stat)
 {
struct stat st;
int r;
@@ -823,7 +819,7 @@ static int add_excludes(const char *fname, const char 
*base, int baselen,
if (size == 0) {
if (sha1_stat) {
fill_stat_data(_stat->stat, );
-   hashcpy(sha1_stat->sha1, EMPTY_BLOB_SHA1_BIN);
+   oidcpy(_stat->oid, _blob_oid);
sha1_stat->valid = 1;
}
close(fd);
@@ -847,10 +843,11 @@ static int add_excludes(const char *fname, const char 
*base, int baselen,
 !ce_stage(istate->cache[pos]) &&
 ce_uptodate(istate->cache[pos]) &&
 !would_convert_to_git(istate, fname))
-   hashcpy(sha1_stat->sha1,
-   istate->cache[pos]->oid.hash);
+   oidcpy(_stat->oid,
+  >cache[pos]->oid);
else
-   hash_sha1_file(buf, size, "blob", 
sha1_stat->sha1);
+   hash_sha1_file(buf, size, "blob",
+  sha1_stat->oid.hash);
fill_stat_data(_stat->stat, );
sha1_stat->valid = 1;
}
@@ -930,7 +927,7 @@ struct exclude_list *add_exclude_list(struct dir_struct 
*dir,
  * Used to set up core.excludesfile and .git/info/exclude lists.
  */
 static void add_excludes_from_file_1(struct dir_struct *dir, const char *fname,
-struct sha1_stat *sha1_stat)
+struct oid_stat *sha1_stat)
 {
struct exclude_list *el;
/*
@@ -1180,7 +1177,7 @@ static void prep_exclude(struct dir_struct *dir,
 
while 

[PATCH v2 06/14] cache: clear whole hash buffer with oidclr

[Patryk Obara]

As long as GIT_SHA1_RAWSZ is equal to GIT_MAX_RAWSZ there's no problem,
but when new hashing algorithm will be in place this memset will clear
only 20-byte prefix of hash buffer.

Alternatively, hashclr implementation could be adjusted, but this
function is almost removed from codebase already.  Separate
implementation of oidclr prevents potential buffer overrun in case
someone incorrectly used hashclr on object_id in future.

Signed-off-by: Patryk Obara 
---
 cache.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 08f2b81e1b..d5d78d6a51 100644
--- a/cache.h
+++ b/cache.h
@@ -1029,7 +1029,7 @@ static inline void hashclr(unsigned char *hash)
 
 static inline void oidclr(struct object_id *oid)
 {
-   hashclr(oid->hash);
+   memset(oid->hash, 0, GIT_MAX_RAWSZ);
 }
 
 
-- 
2.14.3

[PATCH 0/3] nd/shared-index-fix update

[Nguyễn Thái Ngọc Duy]

This only changes the last patch to correct the test prerequisite and
a couple minor refinements. Interdiff:

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 5494389dbb..d2a8e0312a 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -401,7 +401,7 @@ done <<\EOF
 0642 -rw-r---w-
 EOF
 
-test_expect_success POSIXPERM 'graceful handling splitting index is not 
allowed' '
+test_expect_success SANITY 'graceful handling when splitting index is not 
allowed' '
test_create_repo ro &&
(
cd ro &&
@@ -409,11 +409,11 @@ test_expect_success POSIXPERM 'graceful handling 
splitting index is not allowed'
git update-index --split-index &&
test -f .git/sharedindex.*
) &&
-   test_when_finished "chmod -R u+w ro" &&
-   chmod -R u-w ro &&
cp ro/.git/index new-index &&
+   test_when_finished "chmod u+w ro/.git" &&
+   chmod u-w ro/.git &&
GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index 
&&
-   chmod -R u+w ro &&
+   chmod u+w ro/.git &&
rm ro/.git/sharedindex.* &&
GIT_INDEX_FILE=new-index git ls-files >actual &&
echo initial.t >expected &&

Nguyễn Thái Ngọc Duy (3):
  read-cache.c: change type of "temp" in write_shared_index()
  read-cache.c: move tempfile creation/cleanup out of write_shared_index
  read-cache: don't write index twice if we can't write shared index

 read-cache.c   | 40 ++--
 t/t1700-split-index.sh | 19 +++
 2 files changed, 41 insertions(+), 18 deletions(-)

-- 
2.16.0.47.g3d9b0fac3a

[PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index

[Nguyễn Thái Ngọc Duy]

For one thing, we have more consistent cleanup procedure now and always
keep errno intact.

The real purpose is the ability to break out of write_locked_index()
early when mks_tempfile() fails in the next patch. It's more awkward to
do it if this mks_tempfile() is still inside write_shared_index().

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 read-cache.c | 33 +
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 536086e1fe..c568643f55 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2472,31 +2472,18 @@ static int clean_shared_index_files(const char 
*current_hex)
 }
 
 static int write_shared_index(struct index_state *istate,
- struct lock_file *lock, unsigned flags)
+ struct tempfile **temp)
 {
-   struct tempfile *real_temp;
-   struct tempfile **temp = _temp;
struct split_index *si = istate->split_index;
int ret;
 
-   real_temp = mks_tempfile(git_path("sharedindex_XX"));
-   if (!real_temp) {
-   hashclr(si->base_sha1);
-   return do_write_locked_index(istate, lock, flags);
-   }
-   temp = _temp;
move_cache_to_base_index(istate);
ret = do_write_index(si->base, *temp, 1);
-   if (ret) {
-   delete_tempfile(temp);
+   if (ret)
return ret;
-   }
ret = adjust_shared_perm(get_tempfile_path(*temp));
if (ret) {
-   int save_errno = errno;
error("cannot fix permission bits on %s", 
get_tempfile_path(*temp));
-   delete_tempfile(temp);
-   errno = save_errno;
return ret;
}
ret = rename_tempfile(temp,
@@ -2567,7 +2554,21 @@ int write_locked_index(struct index_state *istate, 
struct lock_file *lock,
new_shared_index = istate->cache_changed & SPLIT_INDEX_ORDERED;
 
if (new_shared_index) {
-   ret = write_shared_index(istate, lock, flags);
+   struct tempfile *temp;
+   int saved_errno;
+
+   temp = mks_tempfile(git_path("sharedindex_XX"));
+   if (!temp) {
+   hashclr(si->base_sha1);
+   ret = do_write_locked_index(istate, lock, flags);
+   } else
+   ret = write_shared_index(istate, );
+
+   saved_errno = errno;
+   if (is_tempfile_active(temp))
+   delete_tempfile();
+   errno = saved_errno;
+
if (ret)
goto out;
}
-- 
2.16.0.47.g3d9b0fac3a

[PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Nguyễn Thái Ngọc Duy]

In a0a967568e ("update-index --split-index: do not split if $GIT_DIR is
read only", 2014-06-13), we tried to make sure we can still write an
index, even if the shared index can not be written.

We did so by just calling 'do_write_locked_index()' just before
'write_shared_index()'.  'do_write_locked_index()' always at least
closes the tempfile nowadays, and used to close or commit the lockfile
if COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was
introduced.  COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of
'write_locked_index()'.

After calling 'write_shared_index()', we call 'write_split_index()',
which calls 'do_write_locked_index()' again, which then tries to use the
closed lockfile again, but in fact fails to do so as it's already
closed. This eventually leads to a segfault.

Make sure to write the main index only once.

[nd: most of the commit message and investigation done by Thomas, I only
tweaked the solution a bit]

Helped-by: Thomas Gummerer 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 read-cache.c   |  5 +++--
 t/t1700-split-index.sh | 19 +++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index c568643f55..c58c0a978a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2561,8 +2561,9 @@ int write_locked_index(struct index_state *istate, struct 
lock_file *lock,
if (!temp) {
hashclr(si->base_sha1);
ret = do_write_locked_index(istate, lock, flags);
-   } else
-   ret = write_shared_index(istate, );
+   goto out;
+   }
+   ret = write_shared_index(istate, );
 
saved_errno = errno;
if (is_tempfile_active(temp))
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index af9b847761..d2a8e0312a 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -401,4 +401,23 @@ done <<\EOF
 0642 -rw-r---w-
 EOF
 
+test_expect_success SANITY 'graceful handling when splitting index is not 
allowed' '
+   test_create_repo ro &&
+   (
+   cd ro &&
+   test_commit initial &&
+   git update-index --split-index &&
+   test -f .git/sharedindex.*
+   ) &&
+   cp ro/.git/index new-index &&
+   test_when_finished "chmod u+w ro/.git" &&
+   chmod u-w ro/.git &&
+   GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index 
&&
+   chmod u+w ro/.git &&
+   rm ro/.git/sharedindex.* &&
+   GIT_INDEX_FILE=new-index git ls-files >actual &&
+   echo initial.t >expected &&
+   test_cmp expected actual
+'
+
 test_done
-- 
2.16.0.47.g3d9b0fac3a

[PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index()

[Nguyễn Thái Ngọc Duy]

This local variable 'temp' will be passed in from the caller in the next
patch. To reduce patch noise, let's change its type now while it's still
a local variable and get all the trival conversion out of the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 read-cache.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..536086e1fe 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2474,30 +2474,32 @@ static int clean_shared_index_files(const char 
*current_hex)
 static int write_shared_index(struct index_state *istate,
  struct lock_file *lock, unsigned flags)
 {
-   struct tempfile *temp;
+   struct tempfile *real_temp;
+   struct tempfile **temp = _temp;
struct split_index *si = istate->split_index;
int ret;
 
-   temp = mks_tempfile(git_path("sharedindex_XX"));
-   if (!temp) {
+   real_temp = mks_tempfile(git_path("sharedindex_XX"));
+   if (!real_temp) {
hashclr(si->base_sha1);
return do_write_locked_index(istate, lock, flags);
}
+   temp = _temp;
move_cache_to_base_index(istate);
-   ret = do_write_index(si->base, temp, 1);
+   ret = do_write_index(si->base, *temp, 1);
if (ret) {
-   delete_tempfile();
+   delete_tempfile(temp);
return ret;
}
-   ret = adjust_shared_perm(get_tempfile_path(temp));
+   ret = adjust_shared_perm(get_tempfile_path(*temp));
if (ret) {
int save_errno = errno;
-   error("cannot fix permission bits on %s", 
get_tempfile_path(temp));
-   delete_tempfile();
+   error("cannot fix permission bits on %s", 
get_tempfile_path(*temp));
+   delete_tempfile(temp);
errno = save_errno;
return ret;
}
-   ret = rename_tempfile(,
+   ret = rename_tempfile(temp,
  git_path("sharedindex.%s", 
sha1_to_hex(si->base->sha1)));
if (!ret) {
hashcpy(si->base_sha1, si->base->sha1);
-- 
2.16.0.47.g3d9b0fac3a

Re: [PATCH] files_initial_transaction_commit(): only unlock if locked

[Mathias Rav]

2018-01-22 10:25 +0100 Michael Haggerty :
> On 01/19/2018 11:14 PM, Junio C Hamano wrote:
> > Jeff King  writes:
> >   
> >> On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:
> >>  
> >>> Running git clone --single-branch --mirror -b TAGNAME previously
> >>> triggered the following error message:
> >>>
> >>>   fatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.
> >>>
> >>> This error condition is handled in files_initial_transaction_commit().
> >>>
> >>> 42c7f7ff9 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 
> >>> 2017-06-23)
> >>> introduced incorrect unlocking in the error path of this function,
> >>> which changes the error message to
> >>>
> >>>   fatal: BUG: packed_refs_unlock() called when not locked
> >>>
> >>> Move the call to packed_refs_unlock() above the "cleanup:" label
> >>> since the unlocking should only be done in the last error path.  
> >>
> >> Thanks, this solution looks correct to me. It's pretty low-impact since
> >> the locking is the second-to-last thing in the function, so we don't
> >> have to re-add the unlock to a bunch of error code paths. But one
> >> alternative would be to just do:
> >>
> >>   if (packed_refs_is_locked(refs))
> >>packed_refs_unlock(refs->packed_ref_store);
> >>
> >> in the cleanup section.  
> > 
> > Yeah, that may be a more future-proof alternative, and just as you
> > said the patch as posted would be sufficient, too.  
> 
> Either solution LGTM. Thanks for finding and fixing this bug.
> 
> But let's also take a step back. The invocation
> 
> git clone --single-branch --mirror -b TAGNAME
> 
> seems curious. Does it even make sense to use `--mirror` and
> `--single-branch` at the same time? What should it do?
> 
> Normally `--mirror` implies (aside from `--bare`) that the remote
> references should be converted 1:1 to local references and should be
> overwritten at every fetch; i.e., the refspec should be set to
> `+refs/*:refs/*`.
> 
> To me the most plausible interpretation of `--mirror --single-branch -b
> BRANCHNAME` would be that the single branch should be fetched and made
> the HEAD, and the refspec should be set to
> `+refs/heads/BRANCHNAME:refs/heads/BRANCHNAME`. It also wouldn't be very
> surprising if it were forbidden to use these options together.
> 
> Currently, we do neither of those things. Instead we fetch that one
> reference (as `refs/heads/BRANCHNAME`) but set the refspec to
> `+refs/*:refs/*`; i.e., the next fetch would fetch all of the history.
> 
> It's even more mind-bending if `-b` is passed a `TAGNAME` rather than a
> `BRANCHNAME`. The documentation says that `-b TAGNAME` "detaches the
> HEAD at that commit in the resulting repository". If `--single-branch -b
> TAGNAME` is used, then the refspec is set to
> `+refs/tags/TAGNAME:refs/tags/TAGNAME`. But what if `--mirror` is also used?
> 
> Currently, this fails, apparently because `--mirror` and `-b TAGNAME`
> each independently try to set `refs/tags/TAGNAME` (presumably to the
> same value). *If* this is a useful use case, we could fix it so that it
> doesn't fail. If not, maybe we should prohibit it explicitly and emit a
> clearer error message.
> 
> Mathias: if you encountered this problem in the real world, what were
> you trying to accomplish? What behavior would you have expected?

I wanted a shallow, single-commit clone of a single tag into a bare
repo. Concretely, I wanted to change the Arch Linux build script for
linux-tools to make a shallow clone of the Linux kernel rather than an
ordinary clone, for a tagname corresponding to a released kernel
version. (Of course, it would have been better to change the build
script to download a tarball instead of using git, but without
knowledge of the build script it seemed easier for me to just change
the git invocation.)

Currently, Arch Linux's build script uses
`git clone --mirror "$url" "$dir"` to clone a remote repo;
a StackExchange post [1] suggested changing this to
`git clone --mirror --depth 1 "$url" "$dir"`. (The post also adds
`--single-branch`, but this is implied by `--depth`.)

[1] https://unix.stackexchange.com/a/203335/220010

Naively I added `-b TAGNAME` to fetch just a single tag, but this
resulted in the error.

If I remove `--mirror`, i.e. invoke `git clone --depth 1 -b TAGNAME`,
then the refspec is `+refs/tags/TAGNAME:refs/tags/TAGNAME` as might be
expected, but this results in a non-bare repo.

If instead I change `--mirror` to `--bare`, i.e. invoke
`git clone --bare --depth 1 -b TAGNAME`, this results in a cloned
repository with no `remote.origin.fetch` refspec at all.
I would expect the refspec in this case to be set to
`+refs/tags/TAGNAME:refs/tags/TAGNAME` (just as with `--mirror`).

/Mathias

Re: [PATCH] files_initial_transaction_commit(): only unlock if locked

[Michael Haggerty]

On 01/19/2018 11:14 PM, Junio C Hamano wrote:
> Jeff King  writes:
> 
>> On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:
>>
>>> Running git clone --single-branch --mirror -b TAGNAME previously
>>> triggered the following error message:
>>>
>>> fatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.
>>>
>>> This error condition is handled in files_initial_transaction_commit().
>>>
>>> 42c7f7ff9 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 
>>> 2017-06-23)
>>> introduced incorrect unlocking in the error path of this function,
>>> which changes the error message to
>>>
>>> fatal: BUG: packed_refs_unlock() called when not locked
>>>
>>> Move the call to packed_refs_unlock() above the "cleanup:" label
>>> since the unlocking should only be done in the last error path.
>>
>> Thanks, this solution looks correct to me. It's pretty low-impact since
>> the locking is the second-to-last thing in the function, so we don't
>> have to re-add the unlock to a bunch of error code paths. But one
>> alternative would be to just do:
>>
>>   if (packed_refs_is_locked(refs))
>>  packed_refs_unlock(refs->packed_ref_store);
>>
>> in the cleanup section.
> 
> Yeah, that may be a more future-proof alternative, and just as you
> said the patch as posted would be sufficient, too.

Either solution LGTM. Thanks for finding and fixing this bug.

But let's also take a step back. The invocation

git clone --single-branch --mirror -b TAGNAME

seems curious. Does it even make sense to use `--mirror` and
`--single-branch` at the same time? What should it do?

Normally `--mirror` implies (aside from `--bare`) that the remote
references should be converted 1:1 to local references and should be
overwritten at every fetch; i.e., the refspec should be set to
`+refs/*:refs/*`.

To me the most plausible interpretation of `--mirror --single-branch -b
BRANCHNAME` would be that the single branch should be fetched and made
the HEAD, and the refspec should be set to
`+refs/heads/BRANCHNAME:refs/heads/BRANCHNAME`. It also wouldn't be very
surprising if it were forbidden to use these options together.

Currently, we do neither of those things. Instead we fetch that one
reference (as `refs/heads/BRANCHNAME`) but set the refspec to
`+refs/*:refs/*`; i.e., the next fetch would fetch all of the history.

It's even more mind-bending if `-b` is passed a `TAGNAME` rather than a
`BRANCHNAME`. The documentation says that `-b TAGNAME` "detaches the
HEAD at that commit in the resulting repository". If `--single-branch -b
TAGNAME` is used, then the refspec is set to
`+refs/tags/TAGNAME:refs/tags/TAGNAME`. But what if `--mirror` is also used?

Currently, this fails, apparently because `--mirror` and `-b TAGNAME`
each independently try to set `refs/tags/TAGNAME` (presumably to the
same value). *If* this is a useful use case, we could fix it so that it
doesn't fail. If not, maybe we should prohibit it explicitly and emit a
clearer error message.

Mathias: if you encountered this problem in the real world, what were
you trying to accomplish? What behavior would you have expected?

Maybe the behavior could be made more sane if there were a way to get
the 1:1 reference mapping that `--mirror` implies without also getting
`--bare` [1]. Suppose there were a `--refspec` option. Then instead of

git clone --mirror --single-branch -b BRANCHNAME

with it's non-obvious semantics, you could prohibit that use and instead
support

git clone --bare
--refspec='+refs/heads/BRANCHNAME:refs/heads/BRANCHNAME'

which seems clearer in its intent, if perhaps not super obvious. Or you
could give `clone` a `--no-fetch` option, which would give the user a
time to intervene between setting up the basic clone config and actually
fetching objects.

Michael


[1] It seems like

git clone --config remote.origin.fetch='+refs/*:refs/*' clone ...

might do it, but that actually ends up setting up two refspecs and
only honoring `+refs/heads/*:refs/remotes/origin/*` for the initial
fetch. Plus it is pretty obscure.

Re: [PATCH/RFC] Merge most test helper programs into a new one "test-tool"

[Duy Nguyen]

Whoops, this patch is over 100KB and will likely be blocked by
vger.kernel.org. I may need to split it and resend later, but I think
the commit message is enough for discussion (the actual changes are
not that interesting anyway). The commit is only available at
https://github.com/pclouds/git/tree/t-helper-all-in-one

On Mon, Jan 22, 2018 at 4:21 PM, Nguyễn Thái Ngọc Duy  wrote:
> Plenty small programs in t/helper are now part of a bigger one called
> test-tool. There are two benefits in merging multiple programs into
> one:
>
> - t/helper consumes less disk space (31MB vs 152MB)
> - link time is reduced (with ccache on and -j1, 16s vs 24s)
>
> The following programs remain standalone because...
>
> - test-line-buffer:
> - test-svn-fe: extra dependencies
>
> - test-fake-ssh: some tests require this to be a single argument,
>   splitting it into 'test-tool fake-ssh' creates new problems
>
> - test-dump-fsmonitor:
> - test-dump-untracked-cache:
> - test-run-command:
> - test-wildmatch: some in-flight topics add or remove call sites. It
>   is simpler to leave them out until the dust settles. Then we can
>   move them to test-tool.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  This may bring joy to Johannes and pain to Junio. I've excluded some
>  programs to reduce merge conflicts on 'pu' (only one conflict left in
>  Makefile due to a new test program). But if some new topics show up
>  and use these programs, it's going to be pain pain pain.
>
>  Makefile|  77 -
>  cache.h |   2 +-
>  name-hash.c |   2 +-
>  t/helper/test-chmtime.c |   3 +-
>  t/helper/test-config.c  |   3 +-
>  t/helper/test-ctype.c   |   3 +-
>  t/helper/test-date.c|   3 +-
>  t/helper/test-delta.c   |   3 +-
>  t/helper/test-drop-caches.c |   3 +-
>  t/helper/test-dump-cache-tree.c |   3 +-
>  t/helper/test-dump-split-index.c|   3 +-
>  t/helper/test-example-decorate.c|   3 +-
>  t/helper/test-genrandom.c   |   3 +-
>  t/helper/test-hashmap.c |   3 +-
>  t/helper/test-index-version.c   |   3 +-
>  t/helper/test-lazy-init-name-hash.c |  13 +-
>  t/helper/test-match-trees.c |   3 +-
>  t/helper/test-mergesort.c   |   3 +-
>  t/helper/test-mktemp.c  |   3 +-
>  t/helper/test-online-cpus.c |   3 +-
>  t/helper/test-parse-options.c   |   3 +-
>  t/helper/test-path-utils.c  |   3 +-
>  t/helper/test-prio-queue.c  |   3 +-
>  t/helper/test-read-cache.c  |   3 +-
>  t/helper/test-ref-store.c   |   3 +-
>  t/helper/test-regex.c   |   3 +-
>  t/helper/test-revision-walking.c|   3 +-
>  t/helper/test-scrap-cache-tree.c|   3 +-
>  t/helper/test-sha1-array.c  |   3 +-
>  t/helper/test-sha1.c|   3 +-
>  t/helper/test-sha1.sh   |   4 +-
>  t/helper/test-sigchain.c|   3 +-
>  t/helper/test-strcmp-offset.c   |   3 +-
>  t/helper/test-string-list.c |   3 +-
>  t/helper/test-submodule-config.c|   3 +-
>  t/helper/test-subprocess.c  |   3 +-
>  t/helper/test-tool.c (new)  |  66 
>  t/helper/test-tool.h (new)  |  39 +
>  t/helper/test-urlmatch-normalization.c  |   3 +-
>  t/helper/test-write-cache.c |   3 +-
>  t/lib-git-p4.sh |   2 +-
>  t/lib-git-svn.sh|   2 +-
>  t/lib-pack.sh   |   2 +-
>  t/perf/p0002-read-cache.sh  |   2 +-
>  t/perf/p0004-lazy-init-name-hash.sh |   8 +-
>  t/perf/p0007-write-cache.sh |   2 +-
>  t/perf/p0071-sort.sh|   2 +-
>  t/perf/p7519-fsmonitor.sh   |  12 +-
>  t/t0005-signals.sh  |   6 +-
>  t/t0006-date.sh |   8 +-
>  t/t0009-prio-queue.sh   |   6 +-
>  t/t0011-hashmap.sh  |   4 +-
>  t/t0013-sha1dc.sh   |   4 +-
>  t/t0021-conversion.sh   |   4 +-
>  t/t0040-parse-options.sh|  68 
>  t/t0060-path-utils.sh   |  60 +++
>  t/t0062-revision-walking.sh |   2 +-
>  t/t0063-string-list.sh  |  48 +++---
>  t/t0064-sha1-array.sh   |  16 +-
>  t/t0065-strcmp-offset.sh|   2 +-
>  t/t0070-fundamental.sh  |   8 +-
>  t/t0090-cache-tree.sh   |  18 +--
>  t/t0110-urlmatch-normalization.sh   | 266 
> 
>  t/t1006-cat-file.sh |   2 +-
>  t/t1050-large.sh|   6 +-
>  t/t1300-repo-config.sh