Re: gc and repack ignore .git/*HEAD when checking reachability

[Junio C Hamano]

Josh Triplett  writes:

> This could result in data loss, if a user expected that having an object
> referenced from those places would protect it from pruning.

Yeah, luckily, nobody expects such.  I do not think any of our
document says nothing other than HEAD like CHERRY_PICK_HEAD is
reachability anchoring point; they are designed to be transient.

Because they are designed to be transient, I do not think there is
any downside (other than the initial start-up cost) to including
them in reachability computation.  Because they are meant to be
transient, the objects anchored by them would be reachable from
other anchoring points anyway.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

gc and repack ignore .git/*HEAD when checking reachability

[Josh Triplett]

The manpage for git gc says:
> git gc tries very hard to be safe about the garbage it collects. In
> particular, it will keep not only objects referenced by your current
> set of branches and tags, but also objects referenced by the index,
> remote-tracking branches, refs saved by git filter-branch in
> refs/original/, or reflogs (which may reference commits in branches
> that were later amended or rewound).

gc, repack, and anything else that uses the machinery in reachable.c
will also check HEAD, to include objects only reachable from a detached
HEAD (which the manpage should document).

However, unreachable.c does not check any other ref that sits directly
in the .git directory, such as MERGE_HEAD, FETCH_HEAD, or
CHERRY_PICK_HEAD.  To test this, try creating a new empty repository
with "git init repo ; cd repo", then use "git fetch URL" to fetch a
repository into FETCH_HEAD, then run "git repack -a -d -f", and then
"git show FETCH_HEAD".  This similarly affects "git gc", which will
unpack all the objects from the pack and leave them loose.

This could result in data loss, if a user expected that having an object
referenced from those places would protect it from pruning.

I think the right fix for this would involve having
mark_reachable_objects in reachable.c add all refs that match
.git/*HEAD, not just .git/HEAD itself.  (I'd suggest matching .git/*HEAD
rather than hardcoding the list of "special" refs, to provide
compatibility with any other tool or future version of git that
introduces another such ref.)  This seems fairly easily done with a new
variant of do_head_ref that includes all such refs, along with a
one-line change to mark_reachable_objects to use it.

Does this seem like a reasonable approach?

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git branch doesn't allow me to forcibly delete branch which was checked out in a now-deleted worktree dir

[Erik Johnson]

On Thu, Jul 07, 2016 at 04:39:26PM -0700, Jacob Keller wrote:

On Thu, Jul 7, 2016 at 11:44 AM, Erik Johnson  wrote:

% git branch -D archive-extracted-xz
error: Cannot delete branch 'archive-extracted-xz' checked out at
'/home/erik/git/salt/archive-extracted-xz'
% test -d /home/erik/git/salt/archive-extracted-xz || echo "directory
doesn't exist"
directory doesn't exist
% git --version
git version 2.9.0

I know that I can just get rid of this error by pruning the worktrees,
but this still seems like incorrect behavior on the part of git branch.
It shouldn't be telling me that the branch is checked out in a directory
that does not exist, that is just factually incorrect.



Until the worktree status is updated git branch probably isn't
checking itself. It might be worth triggering a worktree prune when
doing branch work. Note that some worktrees may be on removable media
or similar, and thus even if the directory doesn't exist right now,
that does not mean it's no longer checked out. There is already
support for setting a worktree as "persistent", but this means git
branch definitely shouldn't just do its own check for non existent
directory.

Thanks,
Jake



I'm not expecting _any_ git branch command to prune worktrees, but a
branch _deletion_ shouldn't fail because git thinks the branch is
checked out in a worktree that doesn't exist anymore. Even in the
scenario where the worktree corresponding to that branch is on removable
media, does it really matter? You're trying to delete the branch.

I feel like this is a recent change in behavior, too. I've been using
worktrees since they were first available in 2.5, and I don't remember
having to prune to be able to delete the branch until recently.


--

-Erik

"For me, it is far better to grasp the universe as it really is than to
persist in delusion, however satisfying and reassuring."  --Carl Sagan



signature.asc
Description: PGP signature

Re: git branch doesn't allow me to forcibly delete branch which was checked out in a now-deleted worktree dir

[Jacob Keller]

On Thu, Jul 7, 2016 at 11:44 AM, Erik Johnson  wrote:
> % git branch -D archive-extracted-xz
> error: Cannot delete branch 'archive-extracted-xz' checked out at
> '/home/erik/git/salt/archive-extracted-xz'
> % test -d /home/erik/git/salt/archive-extracted-xz || echo "directory
> doesn't exist"
> directory doesn't exist
> % git --version
> git version 2.9.0
>
> I know that I can just get rid of this error by pruning the worktrees,
> but this still seems like incorrect behavior on the part of git branch.
> It shouldn't be telling me that the branch is checked out in a directory
> that does not exist, that is just factually incorrect.
>

Until the worktree status is updated git branch probably isn't
checking itself. It might be worth triggering a worktree prune when
doing branch work. Note that some worktrees may be on removable media
or similar, and thus even if the directory doesn't exist right now,
that does not mean it's no longer checked out. There is already
support for setting a worktree as "persistent", but this means git
branch definitely shouldn't just do its own check for non existent
directory.

Thanks,
Jake

> --
>
> -Erik
>
> "For me, it is far better to grasp the universe as it really is than to
> persist in delusion, however satisfying and reassuring."  --Carl Sagan
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] correct ce_compare_data() in a middle of a merge

[Junio C Hamano]

Junio C Hamano  writes:

> I am however not convinced passing the full SHA-1 is a good
> solution.  The make_cache_entry() function may be creating a cache
> entry to stuff in a non-default index (i.e. not "the_index"), but
> its caller does not have a way to tell it which index the resulting
> cache entry will go, and calls refresh_cache_entry(), which always
> assumes that the caller is interested in "the_index", so what
> add_cacheinfo() does by calling make_cache_entry() feels wrong in
> the first place.  Also, the call from update_file_flags() knows that
> the working tree is in sync with the resulting cache entry.
>
> Perhaps update_file_flags() should not even call add_cacheinfo()
> there in the first place?  I wonder if it should just call
> add_file_to_index()---after all, even though the current code
> already knows that 99b633 _is_ the result it wants in the index, it
> still goes to the filesystem and rehashes.
>
> I dunno.  I really do not like that extra sha1 argument added all
> over the callchain by this patch.

While the above still stands (i.e. I really want to see us find a
solution without the extra argument all over), I do not think
add_file_to_index() would work well here, as the stage #2 of the
index is contaminated with CRLF in this case, and getting rid of it
is the whole point of renormalization, I would think.  Of course,
you will get a normalized result if you merged in the other
direction, as your stage #2 would have normalized 99b633 when you
cleanly merge the CRLF version from the other side with
renormalization, and "git add" of the cleanly merged result would
keep the normalized version.

Perhaps that is an acceptable downside.  If your side is not
normalized, and if the merge result is the same as what you already
have, perhaps you deserve to keep that unnormalized result, and
you'd be better off normalizing your tree before doing a merge, if
you do not like the fact that your indexed blobs have CRLF.

> Or did you mean other calls to add_cacheinfo()?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

[Jeff King]

On Thu, Jul 07, 2016 at 03:06:31PM -0700, Stefan Beller wrote:

> > The problem is for hosting sites which serve repositories via git-daemon
> > from untrusted users who have real shell accounts (e.g., you set up
> > git-daemon to run as the "daemon" user serving repositories out of
> > people's home directories; you don't want users to escalate their shell
> > access into running arbitrary code as "daemon").
> 
> I think you would want to lock down the
> hosting site as much as possible and not put untrusted users home
> directories on there? So it is hard for me to imagine you'd go for such a 
> setup
> in practice.

Sure, I think that's a good way to run a hosting site, too. But it
doesn't mean people don't have other needs. kernel.org was run as I
mentioned above for many years.

Another related case: you have a multi-user server where Alice might run
"git fetch server:~bob/repo.git". Alice does not want to run arbitrary
code based on what is in Bob's repo.git. Even if they are in the same
company, it is a poor security practice.

> > But I don't think that case applies here. That is about running
> > upload-pack on an untrusted repository, but your changes here are part
> > of receive-pack. In such a scenario, users should be pushing as
> > themselves via ssh. And if they are not (e.g., the admin set up
> > push-over-smart-http centrally), they are already screwed, as a
> > malicious user could just set up a pre-receive hook.
> 
> I hear that as: "The pre-receive hook itself can do much more
> damage than an oversized push option payload".

Exactly. Or more to the point: we promise nothing here except for
upload-pack, so changes to receive-pack do not have to worry about this
issue at all.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

[Junio C Hamano]

Stefan Beller  writes:

> No, there is no good objective reason. I added it just after the atomic
> flag as that is what I implemented.
>
> Is there a reason for a particular order of capabilities? I always considered
> it a set of strings, i.e. any order is valid and there is no preference in
> which way to put it.

That is correct, but "there is no inherent order or grouping" does
not lead to "hence it is OK to put a new thing at a random place in
the middle."

If a reviewer sees a new thing at some specific point in a
collection, when the collection is understood not to have any
specific order or grouping, it makes the reviewer doubt the
belief--there must be a reason why this was placed not at the end;
otherwise a new thing wouldn't be placed randomly at the middle.

If you place a new thing at the end, it still leaves ambiguity; it
may be there because there is no inherent order or grouping, or the
new one is sorts later than the one that used to be at the last or
somehow related to it.  

> I agree on this for all content that can be modified by the user
> (e.g. files in the work tree such as .gitmodules), but the .git/config
> file cannot be changed remotely. So I wonder how an attack would
> look like for a hosting provider or anyone else?
> We still rely on a sane system and trust /etc/gitconfig
> so we do trust the host/admin but not the user?

It is not "we" do trust; it is "we let host/admin trust itself while
making sure that they can protect themselves from their users".

>> More importantly, if we plan to make this configurable and not make
>> the limit a hardwired constant of the wire protocol, it may be
>> better to advertise push-options capability with the limit, e.g.
>> "push-options=32" (or even "push-options=1024/32"), so that the
>> client side can count and abort early?
>
> Yeah we may want to start out with a strict format here indicating
> the parameters used for evaluating the size.
> So what do these numbers mean?
>
> I assume (and hence I should document that,) that the first (1024)
> is the maximum number of bytes per push option. The second
> number (32) is the number of push options (not the number of pkts,
> as one push option may take more than one pkt if the first number is
> larger than 65k, see the NEEDSWORK comment.)
>
> Do we really need 2 numbers, or could we just have one number
> describing the maximum total size in bytes before the remote rejects
> the connection?

That's for you to decide.  push-options=32 is probably OK but it
will prevent a well-behaving "git push" from catching a request that
will be rejected, if you are basing the receiver side decision on
the other number.

The suggestion was primarily my reaction after seeing the two new
calls to die() on the receiver side, whose message I was not sure
will be given to the user who is pushing, i.e.

> When not going over ssh://, does the user see these messages?

Your "git push" will be collecting the options in a string-list
while parsing the options, so it would be able to check immediately
upon seeing the advertised capability if it will trigger this
protocol error even before making the request, which would be a good
thing to do, and I am reasonably sure we can give a better error
message if we do that on the local side without having the receiver
to tell you (for one thing, we can i18n the local side error
message more easily).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

[Stefan Beller]

On Thu, Jul 7, 2016 at 2:56 PM, Jeff King  wrote:
> On Thu, Jul 07, 2016 at 02:41:37PM -0700, Stefan Beller wrote:
>
>> >> + /* NEEDSWORK: expose the limitations to be configurable. */
>> >> + int max_options = 32;
>> >> +
>> >> + /*
>> >> +  * NEEDSWORK: expose the limitations to be configurable;
>> >> +  * Once the limit can be lifted, include a way for payloads
>> >> +  * larger than one pkt, e.g allow a payload of up to
>> >> +  * LARGE_PACKET_MAX - 1 only, and reserve the last byte
>> >> +  * to indicate whether the next pkt continues with this
>> >> +  * push option.
>> >> +  */
>> >> + int max_size = 1024;
>> >
>> > Good NEEDSWORK comments; perhaps also hint that the configuration
>> > must not come from the repository level configuration file (i.e.
>> > Peff's "scoped configuration" from jk/upload-pack-hook topic)?
>>
>> Ok, I reviewed that series. It is unclear to me how the attack would
>> actually look like in that case.
>>
>> In 20b20a22f8f Jeff writes:
>> > Because we promise that
>> > upload-pack is safe to run in an untrusted repository, we
>> > cannot execute arbitrary code or commands found in the
>> > repository (neither in hooks/, nor in the config).
>>
>> I agree on this for all content that can be modified by the user
>> (e.g. files in the work tree such as .gitmodules), but the .git/config
>> file cannot be changed remotely. So I wonder how an attack would
>> look like for a hosting provider or anyone else?
>> We still rely on a sane system and trust /etc/gitconfig
>> so we do trust the host/admin but not the user?
>
> The problem is for hosting sites which serve repositories via git-daemon
> from untrusted users who have real shell accounts (e.g., you set up
> git-daemon to run as the "daemon" user serving repositories out of
> people's home directories; you don't want users to escalate their shell
> access into running arbitrary code as "daemon").

I think you would want to lock down the
hosting site as much as possible and not put untrusted users home
directories on there? So it is hard for me to imagine you'd go for such a setup
in practice.

>
> But I don't think that case applies here. That is about running
> upload-pack on an untrusted repository, but your changes here are part
> of receive-pack. In such a scenario, users should be pushing as
> themselves via ssh. And if they are not (e.g., the admin set up
> push-over-smart-http centrally), they are already screwed, as a
> malicious user could just set up a pre-receive hook.

I hear that as: "The pre-receive hook itself can do much more
damage than an oversized push option payload".

OK.

>
> IOW, we promise only that upload-pack is safe to run an untrusted repo,
> but not receive-pack.
>
> -Peff

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

[Jeff King]

On Thu, Jul 07, 2016 at 02:41:37PM -0700, Stefan Beller wrote:

> >> + /* NEEDSWORK: expose the limitations to be configurable. */
> >> + int max_options = 32;
> >> +
> >> + /*
> >> +  * NEEDSWORK: expose the limitations to be configurable;
> >> +  * Once the limit can be lifted, include a way for payloads
> >> +  * larger than one pkt, e.g allow a payload of up to
> >> +  * LARGE_PACKET_MAX - 1 only, and reserve the last byte
> >> +  * to indicate whether the next pkt continues with this
> >> +  * push option.
> >> +  */
> >> + int max_size = 1024;
> >
> > Good NEEDSWORK comments; perhaps also hint that the configuration
> > must not come from the repository level configuration file (i.e.
> > Peff's "scoped configuration" from jk/upload-pack-hook topic)?
> 
> Ok, I reviewed that series. It is unclear to me how the attack would
> actually look like in that case.
> 
> In 20b20a22f8f Jeff writes:
> > Because we promise that
> > upload-pack is safe to run in an untrusted repository, we
> > cannot execute arbitrary code or commands found in the
> > repository (neither in hooks/, nor in the config).
> 
> I agree on this for all content that can be modified by the user
> (e.g. files in the work tree such as .gitmodules), but the .git/config
> file cannot be changed remotely. So I wonder how an attack would
> look like for a hosting provider or anyone else?
> We still rely on a sane system and trust /etc/gitconfig
> so we do trust the host/admin but not the user?

The problem is for hosting sites which serve repositories via git-daemon
from untrusted users who have real shell accounts (e.g., you set up
git-daemon to run as the "daemon" user serving repositories out of
people's home directories; you don't want users to escalate their shell
access into running arbitrary code as "daemon").

But I don't think that case applies here. That is about running
upload-pack on an untrusted repository, but your changes here are part
of receive-pack. In such a scenario, users should be pushing as
themselves via ssh. And if they are not (e.g., the admin set up
push-over-smart-http centrally), they are already screwed, as a
malicious user could just set up a pre-receive hook.

IOW, we promise only that upload-pack is safe to run an untrusted repo,
but not receive-pack.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options

[Junio C Hamano]

Stefan Beller  writes:

> But your first patch (2/4) would not yet advertise the capability?
> Or advertise and then just ignoring it?

As I wrote...

>> If I were doing this series, I
>> would probably have done 2/4 first without plumbing it through
>> (i.e. it is sent and accumulated in a string list at the receiver,
>> and then cleared and freed without being used), and then added the
>> processing (i.e. this step) as the second patch.

... I would imagine it would advertise, allow the other side to send,
receive and collect, and then discard (properly) without using.

> It is better for documentation purposes in this patch though. It makes
> the other patch harder as "it allows transmitting push options, but
> in that patch nothing of value is done with them."
>
> So I'll see if I can reorder easily.

It does not matter too much. Let's not spend too much time on the
ordering.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] add a test for push options

[Stefan Beller]

On Thu, Jul 7, 2016 at 1:01 PM, Junio C Hamano  wrote:
> On Thu, Jul 7, 2016 at 12:51 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> The functions `mk_repo_pair` as well as `test_refs` are borrowed from
>>> t5543-atomic-push, with additional hooks installed.
>>>
>>> Signed-off-by: Stefan Beller 
>>> ---
>>>  t/t5544-push-options.sh | 101 
>>> 
>>>  1 file changed, 101 insertions(+)
>>>  create mode 100755 t/t5544-push-options.sh
>>
>> FYI:
>>
>> Applying: add a test for push options
>> Test number t5544 already taken
>>
>
> I'll just move it over to t5545; no need to resend.

I'd resend because of the the first three patches anyway,
so I can include a renamed version of tests, too.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options

[Stefan Beller]

On Thu, Jul 7, 2016 at 1:20 PM, Junio C Hamano  wrote:
>
> What is suboptimal about the structure of the series is that we
> won't bisect down to any of the potential bugs in the above code
> even if we ever see any bug in the future.
> It also does not hint
> where push_options is expected to be read in the code in the
> subsequent patches in the series.  If I were doing this series, I
> would probably have done 2/4 first without plumbing it through
> (i.e. it is sent and accumulated in a string list at the receiver,
> and then cleared and freed without being used), and then added the
> processing (i.e. this step) as the second patch.

But your first patch (2/4) would not yet advertise the capability?
Or advertise and then just ignoring it?

That shadows other bugs that would not properly bisect, I'd imagine?

It is better for documentation purposes in this patch though. It makes
the other patch harder as "it allows transmitting push options, but
in that patch nothing of value is done with them."

So I'll see if I can reorder easily.

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

[Stefan Beller]

On Thu, Jul 7, 2016 at 1:37 PM, Junio C Hamano  wrote:
>
>> @@ -207,6 +214,8 @@ static void show_ref(const char *path, const unsigned 
>> char *sha1)
>> "report-status delete-refs side-band-64k quiet");
>>   if (advertise_atomic_push)
>>   strbuf_addstr(, " atomic");
>> + if (advertise_push_options)
>> + strbuf_addstr(, " push-options");
>>   if (prefer_ofs_delta)
>>   strbuf_addstr(, " ofs-delta");
>>   if (push_cert_nonce)
>
> Hmph, was there a good reason to add it in the middle (contrast to
> the previous addition to the "only possible values are..."
> enumeration)?

No, there is no good objective reason. I added it just after the atomic
flag as that is what I implemented.

Is there a reason for a particular order of capabilities? I always considered
it a set of strings, i.e. any order is valid and there is no preference in
which way to put it.

>
>> +static struct string_list *read_push_options()
>
> static struct string_list *read_push_options(void)
>
>> +{
>> + int i;
>> + struct string_list *ret = xmalloc(sizeof(*ret));
>> + string_list_init(ret, 1);
>> +
>> + /* NEEDSWORK: expose the limitations to be configurable. */
>> + int max_options = 32;
>> +
>> + /*
>> +  * NEEDSWORK: expose the limitations to be configurable;
>> +  * Once the limit can be lifted, include a way for payloads
>> +  * larger than one pkt, e.g allow a payload of up to
>> +  * LARGE_PACKET_MAX - 1 only, and reserve the last byte
>> +  * to indicate whether the next pkt continues with this
>> +  * push option.
>> +  */
>> + int max_size = 1024;
>
> Good NEEDSWORK comments; perhaps also hint that the configuration
> must not come from the repository level configuration file (i.e.
> Peff's "scoped configuration" from jk/upload-pack-hook topic)?

Ok, I reviewed that series. It is unclear to me how the attack would
actually look like in that case.

In 20b20a22f8f Jeff writes:
> Because we promise that
> upload-pack is safe to run in an untrusted repository, we
> cannot execute arbitrary code or commands found in the
> repository (neither in hooks/, nor in the config).

I agree on this for all content that can be modified by the user
(e.g. files in the work tree such as .gitmodules), but the .git/config
file cannot be changed remotely. So I wonder how an attack would
look like for a hosting provider or anyone else?
We still rely on a sane system and trust /etc/gitconfig
so we do trust the host/admin but not the user?

>
>> + for (i = 0; i < max_options; i++) {
>> + char *line;
>> + int len;
>> +
>> + line = packet_read_line(0, );
>> +
>> + if (!line)
>> + break;
>> +
>> + if (len > max_size)
>> + die("protocol error: server configuration allows push "
>> + "options of size up to %d bytes", max_size);
>> +
>> + len = strcspn(line, "\n");
>> + line[len] = '\0';
>> +
>> + string_list_append(ret, line);
>> + }
>> + if (i == max_options)
>> + die("protocol error: server configuration only allows up "
>> + "to %d push options", max_options);
>
> When not going over ssh://, does the user sees these messages?
>
> More importantly, if we plan to make this configurable and not make
> the limit a hardwired constant of the wire protocol, it may be
> better to advertise push-options capability with the limit, e.g.
> "push-options=32" (or even "push-options=1024/32"), so that the
> client side can count and abort early?

Yeah we may want to start out with a strict format here indicating
the parameters used for evaluating the size.
So what do these numbers mean?

I assume (and hence I should document that,) that the first (1024)
is the maximum number of bytes per push option. The second
number (32) is the number of push options (not the number of pkts,
as one push option may take more than one pkt if the first number is
larger than 65k, see the NEEDSWORK comment.)

Do we really need 2 numbers, or could we just have one number
describing the maximum total size in bytes before the remote rejects
the connection?

>
> I wondered how well the extra flush works with the extra framing
> smart-http does to wrap the wire protocol; as I do not see any
> change to the http side, I'd assume that there is no issue.

That's a dangerous assumption of yours, as I did not test the
https side, yet.

>
>> +
>> + return ret;
>> +}
>> +
>>  static const char *parse_pack_header(struct pack_header *hdr)
>>  {
>>   switch (read_pack_header(0, hdr)) {
>> @@ -1773,6 +1829,9 @@ int cmd_receive_pack(int argc, const char **argv, 
>> const char *prefix)
>>   const char *unpack_status = NULL;
>>   struct string_list *push_options = NULL;
>>
>> 

Re: What's cooking in git.git (Jul 2016, #02; Wed, 6)

[Charles Bailey]

On Thu, Jul 07, 2016 at 02:21:28PM -0700, Junio C Hamano wrote:
> Charles Bailey  writes:
> 
> > I just wanted to clarify what was actually fixed. The actual bug that
> > was reported and fixed was the fact that 'git grep' (without --cached)
> > wasn't searching the contents of files in the working tree if the index
> > entry had the "intent to add" bit set.
> 
> Ouch, you are absolutely right.
> 
>  Git does not know what the contents in the index should be for a
>  path added with "git add -N" yet, so "git grep --cached" should not
>  show hits (or show lack of hits, with -L) in such a path, but that
>  logic does not apply to "git grep", i.e. searching in the working
>  tree files.  But we did so by mistake, which has been corrected.
> 
> perhaps?

Yes, that reads like an accurate summary to me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Jul 2016, #02; Wed, 6)

[Junio C Hamano]

Charles Bailey  writes:

> I just wanted to clarify what was actually fixed. The actual bug that
> was reported and fixed was the fact that 'git grep' (without --cached)
> wasn't searching the contents of files in the working tree if the index
> entry had the "intent to add" bit set.

Ouch, you are absolutely right.

 Git does not know what the contents in the index should be for a
 path added with "git add -N" yet, so "git grep --cached" should not
 show hits (or show lack of hits, with -L) in such a path, but that
 logic does not apply to "git grep", i.e. searching in the working
 tree files.  But we did so by mistake, which has been corrected.

perhaps?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Makefile: add NEEDS_LIBRT to optionally link with librt

[Junio C Hamano]

Ronald Wampler  writes:

> I am not sure if this the correction solution. Another option I
> considered was to wrap the EXTLIBS += -lrt is an ifndef NO_RT and only
> defining NO_RT for Mac OS X in config.mak.uname.

That alternative would make the resulting code noisier/uglier with
nested ifdef, I would imagine, but it would be of less impact to the
existing users.  But my gut feeling is that the patch you sent is
probably a better solution for the longer term.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] notes-merge: use O_EXCL to avoid overwriting existing files

[Junio C Hamano]

Jeff King  writes:

> Why do we care that the file exists? Should we instead be using the
> lockfile code to get exclusive access to it? That would also switch us
> to doing the write-to-tempfile-and-rename dance, but that seems like it
> would be a good thing. If we hit a write() error in the code now, we
> leave a partially-written file in the notes worktree.

Yeah, I had the same thought when I saw the change.

> I dunno. From my cursory reading of the code, it seems like we'd never
> really expect this file_exists() to trigger in the first place, so
> perhaps it's not worth thinking too hard about it.

Perhaps.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] push: accept push options

[Junio C Hamano]

Stefan Beller  writes:

> +-L::
> +--push-option::
> + Transmit the given string to the server, which passes them to
> + the pre-receive as well as the post-receive hook. Only C strings
> + containing no new lines are allowed.

This is to affect what happens at the remote end, so I would have
understood "-R".  I also would have understood "-P" as a short-hand
for "--push-option".  What is the justification of "-L"?

What does "C strings" mean?  Did you mean to say "A sequence of
bytes excluding NUL is passed verbatim"?

I do not think I saw anything in the code I reviewed so far that
requires "no LF" limitation.

... Ahh, OK, you want to make sure that push-options are
one-per-line in the push certificate.  While I do not think it is
absolutely necessary, starting with a possibly tighter than
necessary limitation is much better than starting loose and having
to tighten it later.

>   } else {
>   struct transport *transport =
>   transport_get(remote, NULL);
> -
> + if (flags & TRANSPORT_PUSH_OPTIONS)
> + transport->push_options = push_options;

The result would be easier to read without the removal of a blank
that separates decl/defn and stmt here.

> @@ -533,6 +542,7 @@ int cmd_push(int argc, const char **argv, const char 
> *prefix)
> 0, "signed", _cert, "yes|no|if-asked", N_("GPG sign the 
> push"),
> PARSE_OPT_OPTARG, option_parse_push_signed },
>   OPT_BIT(0, "atomic", , N_("request atomic transaction on 
> remote side"), TRANSPORT_PUSH_ATOMIC),
> + OPT_STRING_LIST('o', "push-option", _options, 
> N_("server-specific"), N_("option to transmit")),

Here it seems to expect "-o".  If we really want a short option,
"-o" would probably be OK, as I do not think "git push" wants to
have "send the output to this file" option.

> diff --git a/send-pack.c b/send-pack.c
> index 299d303..c943560 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -260,6 +260,7 @@ static int generate_push_cert(struct strbuf *req_buf,
> const char *push_cert_nonce)
>  {
>   const struct ref *ref;
> + struct string_list_item *item;
>   char *signing_key = xstrdup(get_signing_key());
>   const char *cp, *np;
>   struct strbuf cert = STRBUF_INIT;
> @@ -278,6 +279,12 @@ static int generate_push_cert(struct strbuf *req_buf,
>   strbuf_addf(, "nonce %s\n", push_cert_nonce);
>   strbuf_addstr(, "\n");
>  
> + if (args->push_options) {
> + for_each_string_list_item(item, args->push_options)
> + strbuf_addf(, "push-option %s\n", item->string);
> + strbuf_addstr(, "\n");

Why the extra blank?

I would actually have expected to see

certificate version ...
pusher ...

pushee ...  # optional
nonce ...   # optional
push-option ... # optional
push-option ... # optional

  
...

by adding this between the two lines in the pre-context of this
hunk, i.e.

if (push_cert_nonce[0])
strbuf_addf(, "nonce %s\n", push_cert_nonce);
if (args->push_options)
for_each_string_list_item(item, args->push_options)
strbuf_addf(, "push-option %s\n", item->string);
strbuf_addstr(, "\n");

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too

[Jeff King]

On Thu, Jul 07, 2016 at 10:09:17PM +0300, Kirill Smelkov wrote:

> Starting from 6b8fda2d (pack-objects: use bitmaps when packing objects)
> if a repository has bitmap index, pack-objects can nicely speedup
> "Counting objects" graph traversal phase. That however was done only for
> case when resultant pack is sent to stdout, not written into a file.
> 
> We can teach pack-objects to use bitmap index for initial object
> counting phase when generating resultant pack file too:

I'm not sure this is a good idea in general. When bitmaps are in use, we
cannot fill out the details in the object-packing list as thoroughly. In
particular:

  - we will not compute the same write order (which is based on
traversal order), leading to packs that have less efficient cache
characteristics

  - we don't learn about the filename of trees and blobs, which is going
to make the delta step much less efficient. This might be mitigated
by turning on the bitmap name-hash cache; I don't recall how much
detail pack-objects needs on the name (i.e., the full name versus
just the hash).

There may be other subtle things, too. The general idea of tying the
bitmap use to pack_to_stdout is that you _do_ want to use it for
serving fetches and pushes, but for a full on-disk repack via gc, it's
more important to generate a good pack.

Your use case:

> git-backup extracts many packs on repositories restoration. That was my
> initial motivation for the patch.

Seems to be somewhere in between. I'm not sure I understand how you're
invoking pack-objects here, but I wonder if you should be using
"pack-objects --stdout" yourself.

But even if it is the right thing for your use case to be using bitmaps
to generate an on-disk bitmap, I think we should be making sure it
_doesn't_ trigger when doing a normal repack.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Makefile: add NEEDS_LIBRT to optionally link with librt

[Ronald Wampler]

We unconditionally link with librt, when HAVE_CLOCK_GETTIME is defined.
But clock_gettime() has been available in most libc implementations for
some time now (e.g., for glibc since version 2.17) and no longer
requires linking with librt. Furthermore, commit a6c3c63 (configure.ac:
check for clock_gettime() and CLOCK_MONOTONIC) will automatically
determined which library (libc or librt) is required for linking when
checking for clock_gettime().

The assumption to unconditionally link with librt was OK, since either
almost every Unix-like system provides a version of librt for backwards
compatibility or other systems, namely Windows or OS X, never provided
clock_gettime(). However, in the latest release of OS X (macOS Sierra),
this function has been added to OS X libc version. As a result, when
running the configuration script, HAVE_CLOCK_GETTIME is set and since
librt is not present, it causes a linker error.

This patches requires those not building via the configuration scripts
to define NEEDS_LIBRT in addition to HAVE_CLOCK_GETTIME, if needed.

Signed-off-by: Ronald Wampler 
---
I am not sure if this the correction solution. Another option I
considered was to wrap the EXTLIBS += -lrt is an ifndef NO_RT and only
defining NO_RT for Mac OS X in config.mak.uname.

 Makefile | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index de5a030..32f503e 100644
--- a/Makefile
+++ b/Makefile
@@ -351,9 +351,12 @@ all::
 # Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not
 # return NULL when it receives a bogus time_t.
 #
-# Define HAVE_CLOCK_GETTIME if your platform has clock_gettime in librt.
+# Define HAVE_CLOCK_GETTIME if your platform has clock_gettime.
 #
-# Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC in librt.
+# Define HAVE_CLOCK_MONOTONIC if your platform has CLOCK_MONOTONIC.
+#
+# Define NEEDS_LIBRT if your platform requires linking with librt (glibc 
version
+# before 2.17) for clock_gettime and CLOCK_MONOTONIC.
 #
 # Define USE_PARENS_AROUND_GETTEXT_N to "yes" if your compiler happily
 # compiles the following initialization:
@@ -1465,13 +1468,16 @@ endif

 ifdef HAVE_CLOCK_GETTIME
BASIC_CFLAGS += -DHAVE_CLOCK_GETTIME
-   EXTLIBS += -lrt
 endif

 ifdef HAVE_CLOCK_MONOTONIC
BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
 endif

+ifdef NEEDS_LIBRT
+   EXTLIBS += -lrt
+endif
+
 ifdef HAVE_BSD_SYSCTL
BASIC_CFLAGS += -DHAVE_BSD_SYSCTL
 endif
--
2.8.2.874.gcf4c2cf

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Jul 2016, #02; Wed, 6)

[Charles Bailey]

On Wed, Jul 06, 2016 at 02:39:26PM -0700, Junio C Hamano wrote:
> * nd/ita-cleanup (2016-07-01) 3 commits
>   (merged to 'next' on 2016-07-06 at f15aeba)
>  + grep: fix grepping for "intent to add" files
>  + t7810-grep.sh: fix a whitespace inconsistency
>  + t7810-grep.sh: fix duplicated test name
> 
>  Git does not know what the contents in the index should be for a
>  path added with "git add -N" yet, so "git grep --cached" should not
>  show hits (or show lack of hits, with -L) in such a path.  But we
>  did by mistake, which has been corrected.
> 
>  Will merge to 'master'.

I just wanted to clarify what was actually fixed. The actual bug that
was reported and fixed was the fact that 'git grep' (without --cached)
wasn't searching the contents of files in the working tree if the index
entry had the "intent to add" bit set.

My original proposed fix (reversion of the commit that introduced this
change) caused the re-introduction of behaviour where "i-to-a" files
would be reported with -L --cached which wasn't desired. I think because
we spent most of our energy and discussion on fixing this in a better
way we've lost the intent of the original patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

[Junio C Hamano]

Stefan Beller  writes:

> While documenting
> this, fix a nit in the `receive.advertiseAtomic` wording.
>  
>  receive.advertiseAtomic::
>   By default, git-receive-pack will advertise the atomic push
> - capability to its clients. If you don't want to this capability
> + capability to its clients. If you don't want this capability
> + to be advertised, set this variable to false.
> +
> +receive.advertisePushOptions::
> + By default, git-receive-pack will advertise the push options capability
> + to its clients. If you don't want this capability
>   to be advertised, set this variable to false.

I think we correcting the nit by avoiding passive voice, i.e.

If you don't want to advertise this capability, set this
variable to false.

would make it easier to read.

>  in packet-line format to the client, followed by a flush-pkt.  The only
>  real difference is that the capability listing is different - the only
> -possible values are 'report-status', 'delete-refs' and 'ofs-delta'.
> +possible values are 'report-status', 'delete-refs', 'ofs-delta' and
> +'push-options'.

OK.

> +push-options
> +
> +
> +If the server sends the 'push-options' capability it is capable to accept

Two nits:

 - A comma would make it easier to read.
 - "capable" goes with "of ", while "able" goes with "to ".

i.e. "... capability, it is capable of accepting..."

> +push options after the update commands have been sent. If the pushing client
> +requests this capability, the server will pass the options to the pre and 
> post
> +receive hooks that process this push request.

Missing dashes, i.e. "pre- and post-receive hooks"?

> @@ -207,6 +214,8 @@ static void show_ref(const char *path, const unsigned 
> char *sha1)
> "report-status delete-refs side-band-64k quiet");
>   if (advertise_atomic_push)
>   strbuf_addstr(, " atomic");
> + if (advertise_push_options)
> + strbuf_addstr(, " push-options");
>   if (prefer_ofs_delta)
>   strbuf_addstr(, " ofs-delta");
>   if (push_cert_nonce)

Hmph, was there a good reason to add it in the middle (contrast to
the previous addition to the "only possible values are..."
enumeration)?

> +static struct string_list *read_push_options()

static struct string_list *read_push_options(void)

> +{
> + int i;
> + struct string_list *ret = xmalloc(sizeof(*ret));
> + string_list_init(ret, 1);
> +
> + /* NEEDSWORK: expose the limitations to be configurable. */
> + int max_options = 32;
> +
> + /*
> +  * NEEDSWORK: expose the limitations to be configurable;
> +  * Once the limit can be lifted, include a way for payloads
> +  * larger than one pkt, e.g allow a payload of up to
> +  * LARGE_PACKET_MAX - 1 only, and reserve the last byte
> +  * to indicate whether the next pkt continues with this
> +  * push option.
> +  */
> + int max_size = 1024;

Good NEEDSWORK comments; perhaps also hint that the configuration
must not come from the repository level configuration file (i.e.
Peff's "scoped configuration" from jk/upload-pack-hook topic)?

> + for (i = 0; i < max_options; i++) {
> + char *line;
> + int len;
> +
> + line = packet_read_line(0, );
> +
> + if (!line)
> + break;
> +
> + if (len > max_size)
> + die("protocol error: server configuration allows push "
> + "options of size up to %d bytes", max_size);
> +
> + len = strcspn(line, "\n");
> + line[len] = '\0';
> +
> + string_list_append(ret, line);
> + }
> + if (i == max_options)
> + die("protocol error: server configuration only allows up "
> + "to %d push options", max_options);

When not going over ssh://, does the user sees these messages?

More importantly, if we plan to make this configurable and not make
the limit a hardwired constant of the wire protocol, it may be
better to advertise push-options capability with the limit, e.g.
"push-options=32" (or even "push-options=1024/32"), so that the
client side can count and abort early?

I wondered how well the extra flush works with the extra framing
smart-http does to wrap the wire protocol; as I do not see any
change to the http side, I'd assume that there is no issue.

> +
> + return ret;
> +}
> +
>  static const char *parse_pack_header(struct pack_header *hdr)
>  {
>   switch (read_pack_header(0, hdr)) {
> @@ -1773,6 +1829,9 @@ int cmd_receive_pack(int argc, const char **argv, const 
> char *prefix)
>   const char *unpack_status = NULL;
>   struct string_list *push_options = NULL;
>  
> + if (use_push_options)
> + push_options = read_push_options();
> +
>   

Re: [PATCH] notes-merge: use O_EXCL to avoid overwriting existing files

[Jeff King]

On Thu, Jul 07, 2016 at 10:08:30PM +0200, René Scharfe wrote:

> Use the open(2) flag O_EXCL to ensure the file doesn't already exist
> instead of (racily) calling stat(2) through file_exists().  While at it
> switch to xopen() to reduce code duplication and get more consistent
> error messages.

This is definitely an improvement, as it behaves the same except for the
TOCTOU race. But not being very familiar with the notes-merge code, I
have to wonder if this is a system of a larger design issue.

Why do we care that the file exists? Should we instead be using the
lockfile code to get exclusive access to it? That would also switch us
to doing the write-to-tempfile-and-rename dance, but that seems like it
would be a good thing. If we hit a write() error in the code now, we
leave a partially-written file in the notes worktree.

I dunno. From my cursory reading of the code, it seems like we'd never
really expect this file_exists() to trigger in the first place, so
perhaps it's not worth thinking too hard about it.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] am: ignore return value of write_file()

[Jeff King]

On Thu, Jul 07, 2016 at 10:02:14PM +0200, René Scharfe wrote:

> write_file() either returns 0 or dies, so there is no point in checking
> its return value.  The callers of the wrappers write_state_text(),
> write_state_count() and write_state_bool() consequently already ignore
> their return values.  Stop pretenting we care and make them void.

Makes sense. Originally it took "fatal" as a parameter, but it was split
into two functions in 12d6ce1 (write_file(): drop "fatal" parameter,
2015-08-24). The return value on the non-gentle version could have been
dropped at that point.

Arguably we could get rid of the gentle form entirely, as below. The
diffstat is certainly pleasing, but maybe we would eventually want it
for another caller. I dunno. I won't be offended if we drop this as
churn.

-- >8 --
Subject: [PATCH] write_file: drop "gently" form

We have two forms of write_file(): one that dies, and one
that returns an error. However, the latter has only a single
caller, which immediately dies anyway (after producing a
message that is not really any more informative than
write_file's generic die(), and arguably worse because it
does not give the actual filename).

Let's convert that site to use the non-gentle form. At that
point the gentle form has no callers, and we can simplify
the implementation of write_file.

Signed-off-by: Jeff King 
---
As a fun aside, this patch was generated using "--patience",
which gives a much closer to result to what I actually
changed than Myers diff (not meaningful to the patch, but
I'm just always on the lookout for cases where the
algorithms produce meaningfully different results).

 builtin/branch.c |  5 +
 cache.h  |  3 +--
 wrapper.c| 56 
 3 files changed, 14 insertions(+), 50 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 2ecde53..15232c4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -618,10 +618,7 @@ static int edit_branch_description(const char *branch_name)
"  %s\n"
"Lines starting with '%c' will be stripped.\n",
branch_name, comment_line_char);
-   if (write_file_gently(git_path(edit_description), "%s", buf.buf)) {
-   strbuf_release();
-   return error_errno(_("could not write branch description 
template"));
-   }
+   write_file(git_path(edit_description), "%s", buf.buf);
strbuf_reset();
if (launch_editor(git_path(edit_description), , NULL)) {
strbuf_release();
diff --git a/cache.h b/cache.h
index f1dc289..3f6c53f 100644
--- a/cache.h
+++ b/cache.h
@@ -1745,8 +1745,7 @@ static inline ssize_t write_str_in_full(int fd, const 
char *str)
return write_in_full(fd, str, strlen(str));
 }
 
-extern int write_file(const char *path, const char *fmt, ...);
-extern int write_file_gently(const char *path, const char *fmt, ...);
+extern void write_file(const char *path, const char *fmt, ...);
 
 /* pager.c */
 extern void setup_pager(void);
diff --git a/wrapper.c b/wrapper.c
index 5dc4e15..0349441 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -640,56 +640,24 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
return len;
 }
 
-static int write_file_v(const char *path, int fatal,
-   const char *fmt, va_list params)
+void write_file(const char *path, const char *fmt, ...)
 {
+   va_list params;
struct strbuf sb = STRBUF_INIT;
int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
-   if (fd < 0) {
-   if (fatal)
-   die_errno(_("could not open %s for writing"), path);
-   return -1;
-   }
+   if (fd < 0)
+   die_errno(_("could not open %s for writing"), path);
+
+   va_start(params, fmt);
strbuf_vaddf(, fmt, params);
+   va_end(params);
+
strbuf_complete_line();
-   if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
-   int err = errno;
-   close(fd);
-   strbuf_release();
-   errno = err;
-   if (fatal)
-   die_errno(_("could not write to %s"), path);
-   return -1;
-   }
+   if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+   die_errno(_("could not write to %s"), path);
strbuf_release();
-   if (close(fd)) {
-   if (fatal)
-   die_errno(_("could not close %s"), path);
-   return -1;
-   }
-   return 0;
-}
-
-int write_file(const char *path, const char *fmt, ...)
-{
-   int status;
-   va_list params;
-
-   va_start(params, fmt);
-   status = write_file_v(path, 1, fmt, params);
-   va_end(params);
-   return status;
-}
-
-int write_file_gently(const char *path, const char *fmt, ...)
-{
-   int status;
-   va_list params;
-
-   va_start(params, fmt);
-   status = 

Re: [PATCH 1/4] push options: {pre,post}-receive hook learns about push options

[Junio C Hamano]

Stefan Beller  writes:

> As we first want to focus on getting simple strings to work
> reliably, we go with the last option for now.

OK.

> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index d82e912..c875cde 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -247,6 +247,9 @@ Both standard output and standard error output are 
> forwarded to
>  'git send-pack' on the other end, so you can simply `echo` messages
>  for the user.
>  
> +The number of push options are available in the variable 
> GIT_PUSH_OPTION_COUNT
> +and the options itself are in GIT_PUSH_OPTION_{0,1,...}.
> +
>  [[update]]
>  update
>  ~~
> @@ -322,6 +325,9 @@ a sample script `post-receive-email` provided in the 
> `contrib/hooks`
>  directory in Git distribution, which implements sending commit
>  emails.
>  
> +The number of push options are available in the variable 
> GIT_PUSH_OPTION_COUNT
> +and the options itself are in GIT_PUSH_OPTION_{0,1,...}.
> +

The mention of "push options" in these two entries sounded a bit too
abrupt, at least to me.  Perhaps add a short phrase before the
sentence, e.g.

When 'git push --push-option=...' is used, the number of push
options are avaiable ...

or

The number of push options given on the command line of "git
push --push-option=..." can be read from the environment
variable GIT_PUSH_OPTION_COUNT, and the options themselves are
found in GIT_PUSH_OPTION_0, GIT_PUSH_OPTION_1,...

We can read any of the above variants in two ways to describe what
should happen when "git push" is run without "--push-option=...".
Is GIT_PUSH_OPTION_COUNT unset, or set to 0?  Either way, it may be
better to be a bit more explicit.

The hook driver code does not explicitly clear these environment
variables; it is safe to assume that these won't seep in from the
environment, I think, but I am not sure.

> @@ -1756,6 +1771,7 @@ int cmd_receive_pack(int argc, const char **argv, const 
> char *prefix)
>  
>   if ((commands = read_head_info()) != NULL) {
>   const char *unpack_status = NULL;
> + struct string_list *push_options = NULL;
>  
>   prepare_shallow_info(, );
>   if (!si.nr_ours && !si.nr_theirs)
> @@ -1764,13 +1780,19 @@ int cmd_receive_pack(int argc, const char **argv, 
> const char *prefix)
>   unpack_status = unpack_with_sideband();
>   update_shallow_info(commands, , );
>   }
> - execute_commands(commands, unpack_status, );
> + execute_commands(commands, unpack_status, ,
> +  push_options);
>   if (pack_lockfile)
>   unlink_or_warn(pack_lockfile);
>   if (report_status)
>   report(commands, unpack_status);
> - run_receive_hook(commands, "post-receive", 1);
> + run_receive_hook(commands, "post-receive", 1,
> +  push_options);
>   run_update_post_hook(commands);
> + if (push_options) {
> + string_list_clear(push_options, 0);
> + free(push_options);
> + }
>   if (auto_gc) {
>   const char *argv_gc_auto[] = {
>   "gc", "--auto", "--quiet", NULL,
> diff --git a/templates/hooks--pre-receive.sample 
> b/templates/hooks--pre-receive.sample
> new file mode 100644
> index 000..e4d3edc
> --- /dev/null
> +++ b/templates/hooks--pre-receive.sample
> @@ -0,0 +1,22 @@
> +#!/bin/sh
> +#
> +# An example hook script to make use of push options.
> +# The example simply echoes all push options that start with 'echoback='
> +# and rejects all pushes when the "reject" push option is used.
> +#
> +# To enable this hook, rename this file to "pre-receive".
> +
> +if test -n "$GIT_PUSH_OPTION_COUNT"; then
> + i=0
> + while test "$i" -lt "$GIT_PUSH_OPTION_COUNT"; do


Style:

if test -n "$GIT_PUSH_OPTION_COUNT"
then
...

while test ...
do
...

> + eval "value=\$GIT_PUSH_OPTION_$i"
> + case "$value" in
> + echoback=*)
> + echo "echo from the pre-receive-hook ${value#*=}" >&2
> + ;;
> + reject)
> + exit 1
> + esac
> + i=$((i + 1))
> + done
> +fi

What is suboptimal about the structure of the series is that we
won't bisect down to any of the potential bugs in the above code
even if we ever see any bug in the future.  It also does not hint
where push_options is expected to be read in the code in the
subsequent patches in the series.  If I were doing this series, I
would probably have done 2/4 first without plumbing it through
(i.e. it is sent and accumulated in a string list at the receiver,
and then cleared and freed without 

[PATCH] .gitattributes: set file type for C files

[René Scharfe]

Set the diff attribute for C source file to "cpp" in order to improve
git's ability to determine hunk headers.  In particular it helps avoid
showing unindented labels in hunk headers.  That in turn is useful for
git diff -W and git grep -W, which show whole functions now instead of
stopping at a label.

Signed-off-by: Rene Scharfe 
---
 .gitattributes | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitattributes b/.gitattributes
index 5e98806..320e33c 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -1,3 +1,3 @@
 * whitespace=!indent,trail,space
-*.[ch] whitespace=indent,trail,space
+*.[ch] whitespace=indent,trail,space diff=cpp
 *.sh whitespace=indent,trail,space
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] notes-merge: use O_EXCL to avoid overwriting existing files

[René Scharfe]

Use the open(2) flag O_EXCL to ensure the file doesn't already exist
instead of (racily) calling stat(2) through file_exists().  While at it
switch to xopen() to reduce code duplication and get more consistent
error messages.

Signed-off-by: Rene Scharfe 
---
 notes-merge.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/notes-merge.c b/notes-merge.c
index b7814c9..2b29fc4 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -298,12 +298,8 @@ static void write_buf_to_worktree(const unsigned char *obj,
char *path = git_pathdup(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
if (safe_create_leading_directories_const(path))
die_errno("unable to create directory for '%s'", path);
-   if (file_exists(path))
-   die("found existing file at '%s'", path);
 
-   fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, 0666);
-   if (fd < 0)
-   die_errno("failed to open '%s'", path);
+   fd = xopen(path, O_WRONLY | O_EXCL | O_CREAT, 0666);
 
while (size > 0) {
long ret = write_in_full(fd, buf, size);
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] am: ignore return value of write_file()

[René Scharfe]

write_file() either returns 0 or dies, so there is no point in checking
its return value.  The callers of the wrappers write_state_text(),
write_state_count() and write_state_bool() consequently already ignore
their return values.  Stop pretenting we care and make them void.

Signed-off-by: Rene Scharfe 
---
 builtin/am.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index d5da5fe..339e360 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -184,22 +184,22 @@ static inline const char *am_path(const struct am_state 
*state, const char *path
 /**
  * For convenience to call write_file()
  */
-static int write_state_text(const struct am_state *state,
-   const char *name, const char *string)
+static void write_state_text(const struct am_state *state,
+const char *name, const char *string)
 {
-   return write_file(am_path(state, name), "%s", string);
+   write_file(am_path(state, name), "%s", string);
 }
 
-static int write_state_count(const struct am_state *state,
-const char *name, int value)
+static void write_state_count(const struct am_state *state,
+ const char *name, int value)
 {
-   return write_file(am_path(state, name), "%d", value);
+   write_file(am_path(state, name), "%d", value);
 }
 
-static int write_state_bool(const struct am_state *state,
-   const char *name, int value)
+static void write_state_bool(const struct am_state *state,
+const char *name, int value)
 {
-   return write_state_text(state, name, value ? "t" : "f");
+   write_state_text(state, name, value ? "t" : "f");
 }
 
 /**
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] add a test for push options

[Junio C Hamano]

On Thu, Jul 7, 2016 at 12:51 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> The functions `mk_repo_pair` as well as `test_refs` are borrowed from
>> t5543-atomic-push, with additional hooks installed.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>  t/t5544-push-options.sh | 101 
>> 
>>  1 file changed, 101 insertions(+)
>>  create mode 100755 t/t5544-push-options.sh
>
> FYI:
>
> Applying: add a test for push options
> Test number t5544 already taken
>

I'll just move it over to t5545; no need to resend.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] add a test for push options

[Junio C Hamano]

Stefan Beller  writes:

> The functions `mk_repo_pair` as well as `test_refs` are borrowed from
> t5543-atomic-push, with additional hooks installed.
>
> Signed-off-by: Stefan Beller 
> ---
>  t/t5544-push-options.sh | 101 
> 
>  1 file changed, 101 insertions(+)
>  create mode 100755 t/t5544-push-options.sh

FYI:

Applying: add a test for push options
Test number t5544 already taken

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too

[Kirill Smelkov]

Starting from 6b8fda2d (pack-objects: use bitmaps when packing objects)
if a repository has bitmap index, pack-objects can nicely speedup
"Counting objects" graph traversal phase. That however was done only for
case when resultant pack is sent to stdout, not written into a file.

We can teach pack-objects to use bitmap index for initial object
counting phase when generating resultant pack file too:

- if we know bitmap index generation is not enabled for resultant pack:

  Current code has singleton bitmap_git so cannot work simultaneously
  with two bitmap indices.

- if we keep pack reuse enabled still only for "send-to-stdout" case:

  Because on pack reuse raw entries are directly written out to destination
  pack by write_reused_pack() bypassing needed for pack index generation
  bookkeeping done by regular codepath in write_one() and friends.

  (at least that's my understanding after briefly looking at the code)

We also need to care and teach add_object_entry_from_bitmap() to respect
--local via not adding nonlocal loose object to resultant pack (this
is bitmap-codepath counterpart of daae0625 (pack-objects: extend --local
to mean ignore non-local loose objects too) -- not to break 'loose
objects in alternate ODB are not repacked' in t7700-repack.sh .

Otherwise all git tests pass, and for pack-objects -> file we get nice
speedup:

erp5.git[1] (~230MB) extracted from ~ 5GB lab.nexedi.com backup
repository managed by git-backup[2] via

time echo 0186ac99 | git pack-objects --revs erp5pack

before:  37.2s
after:   26.2s

And for `git repack -adb` packed git.git

time echo 5c589a73 | git pack-objects --revs gitpack

before:   7.1s
after:3.6s

i.e. it can be 30% - 50% speedup for pack extraction.

git-backup extracts many packs on repositories restoration. That was my
initial motivation for the patch.

[1] https://lab.nexedi.com/nexedi/erp5
[2] https://lab.nexedi.com/kirr/git-backup

Cc: Vicent Marti 
Cc: Jeff King 
Signed-off-by: Kirill Smelkov 
---
 builtin/pack-objects.c  | 7 +--
 t/t5310-pack-bitmaps.sh | 9 +
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a2f8cfd..be0ebe8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1052,6 +1052,9 @@ static int add_object_entry_from_bitmap(const unsigned 
char *sha1,
 {
uint32_t index_pos;
 
+   if (local && has_loose_object_nonlocal(sha1))
+   return 0;
+
if (have_duplicate_entry(sha1, 0, _pos))
return 0;
 
@@ -2488,7 +2491,7 @@ static int get_object_list_from_bitmap(struct rev_info 
*revs)
if (prepare_bitmap_walk(revs) < 0)
return -1;
 
-   if (pack_options_allow_reuse() &&
+   if (pack_options_allow_reuse() && pack_to_stdout &&
!reuse_partial_packfile_from_bitmap(
_packfile,
_packfile_objects,
@@ -2773,7 +2776,7 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
if (!rev_list_all || !rev_list_reflog || !rev_list_index)
unpack_unreachable_expiration = 0;
 
-   if (!use_internal_rev_list || !pack_to_stdout || 
is_repository_shallow())
+   if (!use_internal_rev_list || (!pack_to_stdout && write_bitmap_index) 
|| is_repository_shallow())
use_bitmap_index = 0;
 
if (pack_to_stdout || !rev_list_all)
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..533fc31 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -118,6 +118,15 @@ test_expect_success 'incremental repack can disable 
bitmaps' '
git repack -d --no-write-bitmap-index
 '
 
+test_expect_success 'pack-objects to file can use bitmap' '
+   # make sure we still have 1 bitmap index from previous tests
+   ls .git/objects/pack/ | grep bitmap >output &&
+   test_line_count = 1 output &&
+   # pack-objects uses bitmap index by default, when it is available
+   packsha1=$(git pack-objects --all mypack output &&
-- 
2.9.0.431.gb11dac7.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Add very verbose porcelain output to status

[Jeff Hostetler]

Tools interacting with Git repositories may need to know the complete
state of the working directory. For efficiency, it would be good to have
a single command to obtain this information.

We already have a `--porcelain` mode intended for tools' consumption,
and it only makes sense to enhance this mode to offer more information.

Just like we do elsewhere in Git's source code, we now interpret
multiple `--verbose` flags accumulatively, and show substantially more
information in porcelain mode at verbosity level 2.

Signed-off-by: Jeff Hostetler 
---
 Documentation/git-status.txt |  83 ++
 builtin/commit.c |  15 +
 t/t7064-wtstatus-vvp.sh  | 150 ++
 wt-status.c  | 676 ++-
 wt-status.h  |  10 +
 5 files changed, 932 insertions(+), 2 deletions(-)
 create mode 100755 t/t7064-wtstatus-vvp.sh

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index e1e8f57..f88316a 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -49,6 +49,9 @@ OPTIONS
twice, then also show the changes in the working tree that
have not yet been staged (i.e., like the output of `git diff`).
 
+   When given twice with `--porcelain`, additional output is enabled.
+   See the section entitled "Very Verbose Porcelain Format" for details.
+
 -u[]::
 --untracked-files[=]::
Show untracked files.
@@ -207,6 +210,86 @@ field from the first filename).  Third, filenames 
containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Very Verbose Porcelain Format
+~
+
+When --verbose is given twice along with --porcelain, additional output
+is provided.
+
+If --branch is given, the first line shows a summary of the current
+operation in progress.  This line begins with "### state: ", the name
+of the operation in progress, and then operation-specific information.
+Fields are separated by a single space.
+
+Operation   Fields Explanation
+--
+clean
+--
+merge  Number unmerged
+--
+am  [E]Present if the current patch
+   is empty
+--
+rebase Number unmerged
+[S]Present if split commit in
+   progress during rebase
+[E]Present if editing a commit
+   during rebase
+[I(/)]Present if in an interactive
+   rebase. Step counts are given.
+[:] Rebase branches
+--
+cherrypick  
+   Number unmerged
+--
+revert  
+   Number unmerged
+--
+bisect  []
+--
+
+If --branch is given, the second line shows branch tracking information.
+This line begins with "### track: ".  Fields are separated by a single
+space, unless otherwise indicated.
+
+FieldMeaning
+
+ | "(initial)"  Current commit
+ | "(detached)"  Current branch
+":"Upstream branch, if set
+"+"   Ahead count, if upstream present
+"-"  Behind count, if upstream present
+
+
+A series of lines are then displayed for the tracked entries.
+Lines have one of the following formats:
+
+XYS mH mI mW shaH shaI PATH
+XYS mH mI mW shaH shaI score OLD_PATH\tPATH
+XYS m1 m2 m3 mW sha1 sha2 sha3 PATH
+
+* X and Y were described in the short format section.
+* S is a one character summary of the submodule status with values '0'..'7'
+  representing the sum of: 4 when submodule has a new commit, 2 when the
+  submodule is modified, and 1 when the submodule contains untracked changes.
+  The value is ' ' if the entry is not a submodule.
+* mH, mI, and mW are the 6 digit octal modes for the head, index, and worktree
+  versions of the entry.
+* m1, m2, and m3 are the modes for the stage 1, 2, and 3 versions of the entry
+  when in an unmerged state.
+* shaH and shaI are the 40 character hashes for the head and 

Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

[Junio C Hamano]

Duy Nguyen  writes:

> I'll deal with that separately. Let's focus on cache-tree only this
> time. So how about this on top?

I was hoping that you would limit the scope of the test to check if
write-tree does the right thing.  i.e. not test "git commit", but
test "git write-tree".

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] correct ce_compare_data() in a middle of a merge

[Junio C Hamano]

Torsten Bögershausen  writes:

> This is the callstack:
>
> merge-recursive.c/add_cacheinfo(unsigned int mode, const unsigned char *sha1,
>   const char *path, int stage, int refresh, int options)
> {
>   struct cache_entry *ce;
>   ce = make_cache_entry
>   if (!ce)
>   return error(_("addinfo_cache failed for path '%s'"), path);
>   return add_cache_entry(ce, options);
> }
> #Calls
> read-cache.c/make_cache_entry(path=file sha1=0x99b633 stage=0)
>
>
> struct cache_entry *make_cache_entry(unsigned int mode,
>   const unsigned char *sha1, const char *path, int stage,
>   unsigned int refresh_options)
> {
> [snip]
>   ret = refresh_cache_entry(ce, refresh_options);
>   if (ret != ce)
>   free(ce);
>   return ret;
> }
>
> #Calls
> refresh_cache_ent(_index, ce, options, NULL, NULL);
>
> #Calls
> ie_modified()
>
> #Calls
> read-cache.c/ie_match_stat()
>
> #Calls
> changed |= ce_modified_check_fs(ce, st);
>
> #Calls
> ce_compare_data(path=file sha1=0x99b633)
>
> #Calls
> index_fd(..., ..., ce->name, ...)
> #Note the sha is lost here, the parameter sha is the output.
>
> Deep down, has_cr_in_index(path) will look at ad55e2 instead,
> which is wrong.

The call to add_cacheinfo() is made with 99b633 to record the 3-way
merge result to the index in this callchain:

 merge_trees()
 -> git_merge_trees()
 -> process_renames() # does nothing for this case
 -> process_entry()
-> merge_content()
   -> merge_file_special_markers()
  -> merge_file_1()
 -> merge_3way()
-> ll_merge() # correctly handles the renormalization
 -> write_sha1_file() # this is what gives us 99b633
-> update_file() # this is fed the 99b633 write_sha1_file() computed
   -> update_file_flags()
  -> read_sha1_file() # reads 99b633
  -> convert_to_working_tree()
  -> write_in_full() # updates the working tree
  -> add_cacheinfo() # to record 99b633 at "file" in the index

So refresh() may incorrectly find that 99b633 does not match what is
in the filesystem if it looks at _any_ entry in the index.  We know
we have written the file ourselves, so by definition it should match
;-) Even though I am not sure why that should affect the overall
correctness of the program (because we have written the correct
result to both working tree and to the index), this needs to be
fixed.

I am however not convinced passing the full SHA-1 is a good
solution.  The make_cache_entry() function may be creating a cache
entry to stuff in a non-default index (i.e. not "the_index"), but
its caller does not have a way to tell it which index the resulting
cache entry will go, and calls refresh_cache_entry(), which always
assumes that the caller is interested in "the_index", so what
add_cacheinfo() does by calling make_cache_entry() feels wrong in
the first place.  Also, the call from update_file_flags() knows that
the working tree is in sync with the resulting cache entry.

Perhaps update_file_flags() should not even call add_cacheinfo()
there in the first place?  I wonder if it should just call
add_file_to_index()---after all, even though the current code
already knows that 99b633 _is_ the result it wants in the index, it
still goes to the filesystem and rehashes.

I dunno.  I really do not like that extra sha1 argument added all
over the callchain by this patch.

Or did you mean other calls to add_cacheinfo()?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

git branch doesn't allow me to forcibly delete branch which was checked out in a now-deleted worktree dir

[Erik Johnson]

% git branch -D archive-extracted-xz
error: Cannot delete branch 'archive-extracted-xz' checked out at 
'/home/erik/git/salt/archive-extracted-xz'
% test -d /home/erik/git/salt/archive-extracted-xz || echo "directory doesn't 
exist"
directory doesn't exist
% git --version
git version 2.9.0

I know that I can just get rid of this error by pruning the worktrees,
but this still seems like incorrect behavior on the part of git branch.
It shouldn't be telling me that the branch is checked out in a directory
that does not exist, that is just factually incorrect.

--

-Erik

"For me, it is far better to grasp the universe as it really is than to
persist in delusion, however satisfying and reassuring."  --Carl Sagan



signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

[Duy Nguyen]

On Wed, Jul 06, 2016 at 12:26:19PM -0700, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy   writes:
> 
> > @@ -426,6 +433,15 @@ int cache_tree_update(struct index_state *istate, int 
> > flags)
> > i = update_one(it, cache, entries, "", 0, , flags);
> > if (i < 0)
> > return i;
> > +   /*
> > +* Top dir can become empty if all entries are i-t-a (even
> > +* from subdirs). Note that we do allow to create an empty
> > +* tree from an empty index. Only error when an empty tree is
> > +* a result of the i-t-a thing.
> > +*/
> > +   if (it->entry_count < 0 &&
> > +   !hashcmp(it->sha1, EMPTY_TREE_SHA1_BIN))
> > +   return error(_("cannot build a tree from just intent-to-add 
> > entries"));
> 
> The test would not let you tell between the two possible ways the
> last step "git commit" fails.
> 
> Did it fail due to the protection this change adds (i.e. you should
> be checking if "git write-tree" fails if that is the case we want to
> cover), or did it fail because you recorded an empty tree as the
> root commit without giving the --allow-empty option?
> 
> In any case, I am not sure about the logic in the comment, either.
> "git commit --allow-empty" should be able to create a new commit
> without any files in it, no?

You're right. If an empty index can produce an empty tree, then an
index full of i-t-a entries should also be able to produce an empty
tree.

git-commit not failing when --allow-empty is not given is another
(known) problem, where it miscounts the number of real index entries.
It's not right to "fix" it in here.

I'll deal with that separately. Let's focus on cache-tree only this
time. So how about this on top?

-- 8< --
diff --git a/cache-tree.c b/cache-tree.c
index 75e73d7..2d50640 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -433,15 +433,6 @@ int cache_tree_update(struct index_state *istate, int 
flags)
i = update_one(it, cache, entries, "", 0, , flags);
if (i < 0)
return i;
-   /*
-* Top dir can become empty if all entries are i-t-a (even
-* from subdirs). Note that we do allow to create an empty
-* tree from an empty index. Only error when an empty tree is
-* a result of the i-t-a thing.
-*/
-   if (it->entry_count < 0 &&
-   !hashcmp(it->sha1, EMPTY_TREE_SHA1_BIN))
-   return error(_("cannot build a tree from just intent-to-add 
entries"));
istate->cache_changed |= CACHE_TREE_CHANGED;
return 0;
 }
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index a19f06b..1a01279 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -104,10 +104,17 @@ test_expect_success 'cache-tree does skip dir that 
becomes empty' '
git init ita-in-dir &&
(
cd ita-in-dir &&
+   echo ground >ground &&
+   git add ground &&
mkdir -p 1/2/3 &&
echo 4 >1/2/3/4 &&
git add -N 1/2/3/4 &&
-   test_must_fail git commit -m committed
+   git commit -m committed &&
+   git ls-tree -r HEAD >actual &&
+   cat >expected <<\EOF &&
+100644 blob b649b43b0708f5604ac912f5a15f7da2bad51a1b   ground
+EOF
+   test_cmp expected actual
)
 '
 
-- 8< --

> > +test_expect_success 'cache-tree does skip dir that becomes empty' '
> > +   rm -fr ita-in-dir &&
> > +   git init ita-in-dir &&
> > +   (
> > +   cd ita-in-dir &&
> > +   mkdir -p 1/2/3 &&
> > +   echo 4 >1/2/3/4 &&
> > +   git add -N 1/2/3/4 &&
> > +   test_must_fail git commit -m committed
> > +   )
> > +'
> > +
> >  test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Questions for "Git User's Survey 2016"

[Stefan Beller]

On Thu, Jul 7, 2016 at 8:03 AM, Jakub Narębski  wrote:
> I am thinking about returning to doing the Git User's Survey, and
> I'd like to ask for feedback.

Thanks for doing the survey!

>
> What I'd like to see is how people use Git, which features do they
> use and which tools (maybe there is some great unknown tool there?),
> where they go to find help about Git.
>
> What types of questions should there be in this year survey?
>

Additionally to the categories you listed, I'd like to propose

4. TEMPORAL. We may get feedback if and how much
some pain points were addressed since the last survey.
This can be done by using the very same question we used in
the last questionnaire, and see how much the answers shifted
or by doing more suggestive questions ("Last survey showed,
feature X is not easy to use, how easy is it now? Think of X
in v1.7.5")

AFAICT the last survey was 2012?

As for the content, I think submodules improved a lot
since then, however I do think they could be improved even more.
e.g. one question could be:

What would you think would improve submodules most
if you could only pick one?

* inclusion of submodules into more commands, e.g.
  "git checkout --recurse-submodules"
* support for direct clones of submodules from a host
  (i.e. not going through the .gitmodules file, but trying
  to obtain the submodule from that superprojects
  .git/modules/. This would only work for non
  bare repos)
* more fault tolerance for miss-configured submodules
* more descriptive error messages for miss-configured
  submodules
* ...

This would help to gauge how to best spend developer time
when trying to improve a feature (like submodules)

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] correct ce_compare_data() in a middle of a merge

[Torsten Bögershausen]

On 2016-07-06 16.57, Junio C Hamano wrote:
> Torsten Bögershausen  writes:
> 
>> At some point inside the merge, Git calls read-cache.c/ce_compare_data(),
>>   to check if the path named "file" is clean.
>>   According to the tree information, the path "file" has the sha1 99b633.
>>   #Note:
>>   #sha1 99b633 is "first line\nsame line\n"
> 
> I thought your scenario was that our side had CRLF both in the blob
> and in the working tree.  So this is a different example?  Our side
> has LF in the blob that is checked out with CRLF in the working tree,
> and their side has CRLF in the blob?
> 
That was probably to confuse myself, and the rest of the world,
sorry for confusion.

This is the callstack:

merge-recursive.c/add_cacheinfo(unsigned int mode, const unsigned char *sha1,
const char *path, int stage, int refresh, int options)
{
struct cache_entry *ce;
ce = make_cache_entry
if (!ce)
return error(_("addinfo_cache failed for path '%s'"), path);
return add_cache_entry(ce, options);
}
#Calls
read-cache.c/make_cache_entry(path=file sha1=0x99b633 stage=0)


struct cache_entry *make_cache_entry(unsigned int mode,
const unsigned char *sha1, const char *path, int stage,
unsigned int refresh_options)
{
[snip]
ret = refresh_cache_entry(ce, refresh_options);
if (ret != ce)
free(ce);
return ret;
}

#Calls
refresh_cache_ent(_index, ce, options, NULL, NULL);

#Calls
ie_modified()

#Calls
read-cache.c/ie_match_stat()

#Calls
changed |= ce_modified_check_fs(ce, st);

#Calls
ce_compare_data(path=file sha1=0x99b633)

#Calls
index_fd(..., ..., ce->name, ...)
#Note the sha is lost here, the parameter sha is the output.

Deep down, has_cr_in_index(path) will look at ad55e2 instead,
which is wrong.


>> ce_compare_data() starts the work:
>> OK, lets run index_fd(...,ce->name,...)
>> # index_fd will simulate a "git add"  and return the sha1 (via the sha1 
>> pointer)
>> # after the hashing.
>>
>> # ce_compare_data() will later compare ce->sha1 with the result stored in
>> # the local sha1. That's why a sha1 is in the parameter list.
>> # To return the resulting hash:
>>
>> ce_compare_data() calls index_fd(sha1, ..., ce->name, ...)
>>
>> #Down in index_fd():
>>
>> sha1_file.c/index_fd() calls index_core() (after consulting the attributes)
>> index_core() reads the file from the working tree into memory using
>> read_in_full(), and after that index_mem() calls hash_sha1_file(buf)
>> to calculate the sha1.
>>
>> Before the hashing is done, index_mem() runs
>> convert_to_git(path, buf, size, , SAFE_CRLF_FALSE)
>> to convert  "blobs to git internal format".
>>
>>
>> Here, convert_to_git() consults the .gitattributes (again) to find out that
>> crlf_action is CRLF_AUTO in our case.
>> The "new safer autocrlf handling" says that if a blob as any CR, don't 
>> convert
>> CRLFs at "git add".
>>
>> convert.c/crlf_to_git() starts to do it's job:
>> - look at buf (It has CRLF, conversion may be needed)
>> - consult blob_has_cr()
>>   # Note: Before this patch, has_cr_in_index(path)) was used
>>
>> #Again, before this patch,
>> has_cr_in_index(path) calls read_blob_data_from_cache(path, ) to read the
>> blob into memory.
>>
>> Now read_blob_data_from_cache() is a macro, and we end up in
>> read_blob_data_from_index(_index, (path), (sz))
>>
>> read-cache.c/read_blob_data_from_index() starts its work:
>>  pos = index_name_pos(istate, path, len);
>>  if (pos < 0) {
>>  /*
>>   * We might be in the middle of a merge, in which
>>   * case we would read stage #2 (ours).
>>   */
>>
>> # And here, and this is important to notice, "ours" is sha1 ad55e2,
>> # which corresponds to "first line\r\nsame line\r\n"
> 
> Where did 99b633 come from then?  There still is something missing
> in this description.
> 
> Puzzled...
This is an unfinished attempt for a commit message:
--
correct ce_compare_data() at the end of a merge

The following didn't work as expected:

 - At the end of a merge
 - merge.renormalize is true,
 - .gitattributes = "* text=auto"
 - core.eol = crlf

Merge a blob with CRLF "first line\r\nsame line\r\n" and a blob
with LF "first line\nsame line\n".

The expected result of the merge is "first line\nsame line\n".

The content in the working tree is "first line\r\nsame line\r\n",
and ce_compare_data() should find that the content is clean and return 0.

The following callstack does not work:
merge-recursive.c/add_cacheinfo(path=file sha1=0x99b633)
#Calls
refresh_cache_ent(_index, ce, options, NULL, NULL);

#Calls
ie_modified()

#Calls
read-cache.c/ie_match_stat()

#Calls
changed |= ce_modified_check_fs(ce, st);

#Calls
ce_compare_data(path=file sha1=0x99b633)

#Calls
index_fd(..., ..., ce->name, ...)
#Note the sha is lost here.

#Calls
index_core()

index_core() reads the file 

Re: bug: depth 1 and recursive update of submodules broke in 2.9.0

[Stefan Beller]

See the discussion and bug fix at
http://thread.gmane.org/gmane.comp.version-control.git/297687/focus=297719

This is a known regression in 2.9.0 and the fix is in the master branch already.
(I think Junio will also roll it into 2.9.1)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

bug: depth 1 and recursive update of submodules broke in 2.9.0

[Stephanos]

When I try to ```git clone --depth 1 --recursive
https://github.com/davidhalter/jedi-vim.git``` in 2.9.0, it fails with
the message: ```
Cloning into 'jedi-vim'...
remote: Counting objects: 37, done.
remote: Compressing objects: 100% (27/27), done.
remote: Total 37 (delta 1), reused 16 (delta 0), pack-reused 0
Unpacking objects: 100% (37/37), done.
Checking connectivity... done.
Submodule 'jedi' (https://github.com/davidhalter/jedi.git) registered
for path 'jedi'
Cloning into '/home/stephanos/jedi-vim/jedi'...
error: no such remote ref 995a6531225ba0b65e1ff863d97e5404d989047b
Fetched in submodule path 'jedi', but it did not contain
995a6531225ba0b65e1ff863d97e5404d989047b. Direct fetching of that
commit failed.
```, while with git 2.8.3 it succeeds. The commit
995a6531225ba0b65e1ff863d97e5404d989047b exists.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] am: counteract gender bias

[Junio C Hamano]

Jakub Narębski  writes:

> Also, in all (?) other places we use "ours" and "theirs"; it looks like
> git-am was a strange exception with "ours" and "his" (also, it was/is
> inconsistent in using plural vs singular form).  Though perhaps it was
> created before the terminology solidified...

I also thought that "ours" vs "his" was strange mixture of plural
and singular when I first saw it, but it turns out there was no
mixture.

It originates at 47f0b6d5 (Fall back to three-way merge when
applying a patch., 2005-10-06), where the code used $his_tree and
$orig_tree, (there was no reference to "our" tree), both singular.
They were copied to "git am" introduced at d1c5f2a4 (Add git-am,
applymbox replacement., 2005-10-07) almost verbatim.

The use of "ours" & "theirs" is now established, and I agree with
you that the use of "his" that is contained to the fall_back_3way
helper function (cf. contrib/examples/git-am.sh) is an oddball.

We should just use "theirs" to be consistent from the beginning, as
you suggested.  There is no need to churn the codebase for political
correctness to first use "hers" that everybody knows will *not* be
the final form.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/17] am: counteract gender bias

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Thu, 7 Jul 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>> 
>> >> I doubt this kind fo distraction is desirable in the middle of a
>> >> seriously heavy series like this one.  As a standalone clean-up to
>> >> turn these directly to "their" that everybody would agree on and can
>> >> be merged down quickly to 'master' that does not have to keep the
>> >> body of the main topic waiting for the dust to settle might be a
>> >> better approach.
>> >>  ...
>> > I am really curious, though. Has it not been our practice to encourage
>> > preparatory patches like white-space or const fixes as part of patch
>> > series that touch a certain part of the code that needed fixing?
>> 
>> Yes, isn't that "preparatory clean-up" what I said "might be a
>> better approach"?
>
> What I meant by "preparatory clean-up" is a patch in the patch series,
> just as I had it.
>
> Now it is a separate patch "series".

The main thing I had trouble with was to have it in the middle at
11/17.  Since I do not have time to reorder other people's patches,
every time you reroll the main series, this step will need to be
looked at together with others.  Having it near the beginning as
preparatory clean-up, or even better a separate series that is an
uncontroversial preparatory clean-up that the main topic depends on,
is what I meant.

You build the main "series" on top of what 'master' would have when
the separate one that "does not have to keep the body of the main
topic waiting" gets merged.  Because the separate one would be
something "everybody would agree on and can be merged down quickly",
that will not slow the main topic down.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/17] am: counteract gender bias

[Johannes Schindelin]

Hi Junio,

On Thu, 7 Jul 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> I doubt this kind fo distraction is desirable in the middle of a
> >> seriously heavy series like this one.  As a standalone clean-up to
> >> turn these directly to "their" that everybody would agree on and can
> >> be merged down quickly to 'master' that does not have to keep the
> >> body of the main topic waiting for the dust to settle might be a
> >> better approach.
> >>  ...
> > I am really curious, though. Has it not been our practice to encourage
> > preparatory patches like white-space or const fixes as part of patch
> > series that touch a certain part of the code that needed fixing?
> 
> Yes, isn't that "preparatory clean-up" what I said "might be a
> better approach"?

What I meant by "preparatory clean-up" is a patch in the patch series,
just as I had it.

Now it is a separate patch "series".

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/3] Additional rebase -i tests

[Johannes Schindelin]

This is just another patch series in preparation for the rebase--helper.

Relative to v1 of this patch series,

- the grammo fix was backed out because it was picked up separately,

- two new tests were introduced, one demonstrating a bug, the other one
  ensuring that the rebase--helper does not introduce a regression (this
  test actually helped me debug and fix a regression in some previous
  revision of the rebase--helper)


Johannes Schindelin (3):
  t3404: add a test for the --gpg-sign option
  rebase -i: demonstrate a bug with --autosquash
  rebase -i: we allow extra spaces after fixup!/squash!

 t/t3404-rebase-interactive.sh |  8 
 t/t3415-rebase-autosquash.sh  | 33 +
 2 files changed, 41 insertions(+)

Published-As: https://github.com/dscho/git/releases/tag/rebase-i-tests-v2
Interdiff vs v1:

 diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
 index 4c96075..aa393d2 100755
 --- a/t/t3404-rebase-interactive.sh
 +++ b/t/t3404-rebase-interactive.sh
 @@ -60,7 +60,7 @@ test_expect_success 'setup' '
test_commit P fileP
  '
  
 -# "exec" commands are run with the user shell by default, but this may
 +# "exec" commands are ran with the user shell by default, but this may
  # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
  # to create a file. Unsetting SHELL avoids such non-portable behavior
  # in tests. It must be exported for it to take effect where needed.
 @@ -1281,11 +1281,12 @@ test_expect_success 'editor saves as CR/LF' '
)
  '
  
 -EPIPHANY="'"
 +SQ="'"
  test_expect_success 'rebase -i --gpg-sign=' '
set_fake_editor &&
 -  FAKE_LINES="edit 1" git rebase -i --gpg-sign=\" HEAD^ >out 2>err &&
 -  grep "$EPIPHANY-S\"$EPIPHANY" err
 +  FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
 +  >out 2>err &&
 +  grep "$SQ-S\"S I Gner\"$SQ" err
  '
  
  test_done
 diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
 index 8f53e54..48346f1 100755
 --- a/t/t3415-rebase-autosquash.sh
 +++ b/t/t3415-rebase-autosquash.sh
 @@ -271,4 +271,37 @@ test_expect_success 'autosquash with custom inst format' '
test 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)
  '
  
 +set_backup_editor () {
 +  write_script backup-editor.sh <<-\EOF
 +  cp "$1" .git/backup-"$(basename "$1")"
 +  EOF
 +  test_set_editor "$PWD/backup-editor.sh"
 +}
 +
 +test_expect_failure 'autosquash with multiple empty patches' '
 +  test_tick &&
 +  git commit --allow-empty -m "empty" &&
 +  test_tick &&
 +  git commit --allow-empty -m "empty2" &&
 +  test_tick &&
 +  >fixup &&
 +  git add fixup &&
 +  git commit --fixup HEAD^^ &&
 +  (
 +  set_backup_editor &&
 +  GIT_USE_REBASE_HELPER=false \
 +  git rebase -i --force-rebase --autosquash HEAD~4 &&
 +  grep empty2 .git/backup-git-rebase-todo
 +  )
 +'
 +
 +test_expect_success 'extra spaces after fixup!' '
 +  base=$(git rev-parse HEAD) &&
 +  test_commit to-fixup &&
 +  git commit --allow-empty -m "fixup!  to-fixup" &&
 +  git rebase -i --autosquash --keep-empty HEAD~2 &&
 +  parent=$(git rev-parse HEAD^) &&
 +  test $base = $parent
 +'
 +
  test_done

-- 
2.9.0.278.g1caae67

base-commit: 5c589a73de4394ad125a4effac227b3aec856fa1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] t3404: add a test for the --gpg-sign option

[Johannes Schindelin]

For the upcoming rebase--helper work (which will accelerate the
interactive rebase noticably), it is important to verify that the
--gpg-sign option is handled properly.

Please note that this patch does this on the cheap, by verifying that
the expected option is printed in the message of the 'edit' operation.

We really should test that the interactive rebase signs the commits
properly, iff GPG is available. This test is left for later.

Signed-off-by: Johannes Schindelin 
---
 t/t3404-rebase-interactive.sh | 8 
 1 file changed, 8 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 66348f1..aa393d2 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1281,4 +1281,12 @@ test_expect_success 'editor saves as CR/LF' '
)
 '
 
+SQ="'"
+test_expect_success 'rebase -i --gpg-sign=' '
+   set_fake_editor &&
+   FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
+   >out 2>err &&
+   grep "$SQ-S\"S I Gner\"$SQ" err
+'
+
 test_done
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/3] rebase -i: demonstrate a bug with --autosquash

[Johannes Schindelin]

When rearranging the edit script, we happily mistake the comment
character for a command, and the command for a SHA-1. As a consequence,
when we move fixup! and squash! commits, our logic to skip lines with
already handled SHA-1s mistakenly skips anything but the first
commented-out pick line, too.

The upcoming rebase--helper patches will address this bug, therefore we
do not need to make the current autosquash code even more complex than
it already is, just to fix this bug.

Signed-off-by: Johannes Schindelin 
---
 t/t3415-rebase-autosquash.sh | 24 
 1 file changed, 24 insertions(+)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 8f53e54..9b71a49 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -271,4 +271,28 @@ test_expect_success 'autosquash with custom inst format' '
test 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)
 '
 
+set_backup_editor () {
+   write_script backup-editor.sh <<-\EOF
+   cp "$1" .git/backup-"$(basename "$1")"
+   EOF
+   test_set_editor "$PWD/backup-editor.sh"
+}
+
+test_expect_failure 'autosquash with multiple empty patches' '
+   test_tick &&
+   git commit --allow-empty -m "empty" &&
+   test_tick &&
+   git commit --allow-empty -m "empty2" &&
+   test_tick &&
+   >fixup &&
+   git add fixup &&
+   git commit --fixup HEAD^^ &&
+   (
+   set_backup_editor &&
+   GIT_USE_REBASE_HELPER=false \
+   git rebase -i --force-rebase --autosquash HEAD~4 &&
+   grep empty2 .git/backup-git-rebase-todo
+   )
+'
+
 test_done
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] rebase -i: we allow extra spaces after fixup!/squash!

[Johannes Schindelin]

This new test case ensures that we handle commit messages that start
with fixup! or squash! followed by more than one space. While we do
not generate such messages when committing with --fixup/--squash, it
is perfectly legal for users to hand-craft their own fixup messages,
and we heed Postel's law by being lenient.

Signed-off-by: Johannes Schindelin 
---
 t/t3415-rebase-autosquash.sh | 9 +
 1 file changed, 9 insertions(+)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 9b71a49..48346f1 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -295,4 +295,13 @@ test_expect_failure 'autosquash with multiple empty 
patches' '
)
 '
 
+test_expect_success 'extra spaces after fixup!' '
+   base=$(git rev-parse HEAD) &&
+   test_commit to-fixup &&
+   git commit --allow-empty -m "fixup!  to-fixup" &&
+   git rebase -i --autosquash --keep-empty HEAD~2 &&
+   parent=$(git rev-parse HEAD^) &&
+   test $base = $parent
+'
+
 test_done
-- 
2.9.0.278.g1caae67
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/17] am: counteract gender bias

[Junio C Hamano]

Johannes Schindelin  writes:

>> I doubt this kind fo distraction is desirable in the middle of a
>> seriously heavy series like this one.  As a standalone clean-up to
>> turn these directly to "their" that everybody would agree on and can
>> be merged down quickly to 'master' that does not have to keep the
>> body of the main topic waiting for the dust to settle might be a
>> better approach.
>>  ...
> I am really curious, though. Has it not been our practice to encourage
> preparatory patches like white-space or const fixes as part of patch
> series that touch a certain part of the code that needed fixing?

Yes, isn't that "preparatory clean-up" what I said "might be a
better approach"?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] t3404: add a test for the --gpg-sign option

[Johannes Schindelin]

Hi Junio,

On Wed, 6 Jul 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Okay, so here is the deal: on the development machine where this was
> > developed, I do not have gpg installed. So please take this test case just
> > to make sure that things work as intended for the moment.
> >
> > Before sending the last rebase--helper patch series, I will make sure to
> > add a real test that requires gpg, and submit that, too.
> >
> > Deal?
> 
> I do not particularly care if the latter one happens.
> 
> The only thing I care about is that the earlier round documents that
> we know we probably should test the real driving of the GPG program,
> but we deliberately do not do so in the series, and hint that such
> an enhancement can happen later.
> 
> That might even entice others to help writing a test or two ;-)

Okay. I tried my hand at editing the commit message, and threw two more
tests into the patch series for good measure. Will send out v2 once the
test suite passed (it's still running).

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] Questions for "Git User's Survey 2016"

[Jakub Narębski]

I am thinking about returning to doing the Git User's Survey, and
I'd like to ask for feedback.

Thanks to generocity of Survs.com, we have been gifted with premium
annual plan (previous surveys always generated quite a large number
of responses).  This plan will last (at least; it was usually
automatically renewed at no cost) till 26 October 2016, so I am
planning on having the survey for 1 month, in September:

  1 September -- 30 September 2016

For now I would like to ask what types of questions would you want
to see in the survey, and what you want to get from it (what would
be the survey purpose).

In my opinion, the survey has (at least) threefold purpose:

1. INFORMATIONAL.  We can find out how people use Git, if the range
of people answering the survey is wide enough and diverse enough.
We can find out what are their pain points.  It might be different
from what we here on this mailing list think it is.  Hopefully
this would help direct Git development to make Git better for all.

2. EDUCATIONAL.  For example a question about which features do
one uses, or how one gets help about Git, tends to be educational
for people taking the survey.

3. ADVERTISEMENT.  Announcing Git User's Survey might be considered
advertisement for git mailing list.  Questions about hosting site
or books can also be considered advertisement for the site in question
or the book / the author, respectively.


What I'd like to see is how people use Git, which features do they
use and which tools (maybe there is some great unknown tool there?),
where they go to find help about Git.

What types of questions should there be in this year survey?

-- 
Jakub Narębski

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 16/16] merge-recursive: flush output buffer even when erroring out

[Johannes Schindelin]

Ever since 66a155b (Enable output buffering in merge-recursive.,
2007-01-14), we had a problem: When the merge failed in a fatal way, all
regular output was swallowed because we called die() and did not get a
chance to drain the output buffers.

To fix this, several modifications were necessary:

- we needed to stop die()ing, to give callers a chance to do something
  when an error occurred (in this case, flush the output buffers),

- we needed to delay printing the error message so that the caller can
  print the buffered output before that, and

- we needed to make sure that the output buffers are flushed even when
  the return value indicates an error.

The first two changes were introduced through earlier commits in this
patch series, and this commit addresses the third one.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index c0be0c5..cb701ee 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2083,6 +2083,7 @@ int merge_recursive(struct merge_options *o,
o->ancestor = "merged common ancestors";
clean = merge_trees(o, h1->tree, h2->tree, 
merged_common_ancestors->tree,
);
+   flush_output(o);
if (clean < 0)
return clean;
 
@@ -2091,7 +2092,6 @@ int merge_recursive(struct merge_options *o,
commit_list_insert(h1, &(*result)->parents);
commit_list_insert(h2, &(*result)->parents->next);
}
-   flush_output(o);
if (o->buffer_output < 2)
strbuf_release(>obuf);
if (show(o, 2))
-- 
2.9.0.278.g1caae67
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 15/16] Ensure that the output buffer is released after calling merge_trees()

[Johannes Schindelin]

The recursive merge machinery accumulates its output in an output
buffer, to be flushed at the end of merge_recursive(). At this point,
we forgot to release the output buffer.

When calling merge_trees() (i.e. the non-recursive part of the recursive
merge) directly, the output buffer is never flushed because the caller
may be merge_recursive() which wants to flush the output itself.

For the same reason, merge_trees() cannot release the output buffer: it
may still be needed.

Forgetting to release the output buffer did not matter much when running
git-checkout, or git-merge-recursive, because we exited after the
operation anyway. Ever since cherry-pick learned to pick a commit range,
however, this memory leak had the potential of becoming a problem.

Signed-off-by: Johannes Schindelin 
---
 builtin/checkout.c | 1 +
 merge-recursive.c  | 2 ++
 sequencer.c| 1 +
 3 files changed, 4 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07dea3b..8d852d4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -573,6 +573,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
exit(128);
ret = reset_tree(new->commit->tree, opts, 0,
 writeout_error);
+   strbuf_release();
if (ret)
return ret;
}
diff --git a/merge-recursive.c b/merge-recursive.c
index 10d3355..c0be0c5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2092,6 +2092,8 @@ int merge_recursive(struct merge_options *o,
commit_list_insert(h2, &(*result)->parents->next);
}
flush_output(o);
+   if (o->buffer_output < 2)
+   strbuf_release(>obuf);
if (show(o, 2))
diff_warn_rename_limit("merge.renamelimit",
   o->needed_rename_limit, 0);
diff --git a/sequencer.c b/sequencer.c
index 286a435..ec50519 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -293,6 +293,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
clean = merge_trees(,
head_tree,
next_tree, base_tree, );
+   strbuf_release();
if (clean < 0)
return clean;
 
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 13/16] merge-recursive: write the commit title in one go

[Johannes Schindelin]

In 66a155b (Enable output buffering in merge-recursive., 2007-01-14), we
changed the code such that it prints the output in one go, to avoid
interfering with the progress output.

Let's make sure that the same holds true when outputting the commit
title: previously, we used several printf() statements to stdout and
speculated that stdout's buffer is large enough to hold the entire
commit title.

Apart from making that speculation unnecessary, we change the code to
add the message to the output buffer before flushing for another reason:
the next commit will introduce a new level of output buffering, where
the caller can request the output not to be flushed, but to be retained
for further processing.

This latter feature will be needed when teaching the sequencer to do
rebase -i's brunt work: it wants to control the output of the
cherry-picks (i.e. recursive merges).

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 205ea04..ad5b961 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -191,25 +191,26 @@ static void output(struct merge_options *o, int v, const 
char *fmt, ...)
 
 static void output_commit_title(struct merge_options *o, struct commit *commit)
 {
-   int i;
-   flush_output(o);
-   for (i = o->call_depth; i--;)
-   fputs("  ", stdout);
+   strbuf_addchars(>obuf, ' ', o->call_depth * 2);
if (commit->util)
-   printf("virtual %s\n", merge_remote_util(commit)->name);
+   strbuf_addf(>obuf, "virtual %s\n",
+   merge_remote_util(commit)->name);
else {
-   printf("%s ", find_unique_abbrev(commit->object.oid.hash, 
DEFAULT_ABBREV));
+   strbuf_addf(>obuf, "%s ",
+   find_unique_abbrev(commit->object.oid.hash,
+   DEFAULT_ABBREV));
if (parse_commit(commit) != 0)
-   printf(_("(bad commit)\n"));
+   strbuf_addf(>obuf, _("(bad commit)\n"));
else {
const char *title;
const char *msg = get_commit_buffer(commit, NULL);
int len = find_commit_subject(msg, );
if (len)
-   printf("%.*s\n", len, title);
+   strbuf_addf(>obuf, "%.*s\n", len, title);
unuse_commit_buffer(commit, msg);
}
}
+   flush_output(o);
 }
 
 static int add_cacheinfo(struct merge_options *o,
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 10/16] merge-recursive: switch to returning errors instead of dying

[Johannes Schindelin]

The recursive merge machinery is supposed to be a library function, i.e.
it should return an error when it fails. Originally the functions were
part of the builtin "merge-recursive", though, where it was simpler to
call die() and be done with error handling.

The existing callers were already prepared to detect negative return
values to indicate errors and to behave as previously: exit with code 128
(which is the same thing that die() does, after printing the message).

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 63 +++
 1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 89eb937..7c9f22c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -266,8 +266,10 @@ struct tree *write_tree_from_memory(struct merge_options 
*o)
active_cache_tree = cache_tree();
 
if (!cache_tree_fully_valid(active_cache_tree) &&
-   cache_tree_update(_index, 0) < 0)
-   die(_("error building trees"));
+   cache_tree_update(_index, 0) < 0) {
+   error(_("error building trees"));
+   return NULL;
+   }
 
result = lookup_tree(active_cache_tree->sha1);
 
@@ -707,12 +709,10 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
/* Make sure leading directories are created */
status = safe_create_leading_directories_const(path);
if (status) {
-   if (status == SCLD_EXISTS) {
+   if (status == SCLD_EXISTS)
/* something else exists */
-   error(msg, path, _(": perhaps a D/F conflict?"));
-   return -1;
-   }
-   die(msg, path, "");
+   return error(msg, path, _(": perhaps a D/F conflict?"));
+   return error(msg, path, "");
}
 
/*
@@ -740,6 +740,8 @@ static int update_file_flags(struct merge_options *o,
  int update_cache,
  int update_wd)
 {
+   int ret = 0;
+
if (o->call_depth)
update_wd = 0;
 
@@ -760,9 +762,11 @@ static int update_file_flags(struct merge_options *o,
 
buf = read_sha1_file(oid->hash, , );
if (!buf)
-   die(_("cannot read object %s '%s'"), oid_to_hex(oid), 
path);
-   if (type != OBJ_BLOB)
-   die(_("blob expected for %s '%s'"), oid_to_hex(oid), 
path);
+   return error(_("cannot read object %s '%s'"), 
oid_to_hex(oid), path);
+   if (type != OBJ_BLOB) {
+   ret = error(_("blob expected for %s '%s'"), 
oid_to_hex(oid), path);
+   goto free_buf;
+   }
 
if (make_room_for_path(o, path) < 0) {
update_wd = 0;
@@ -778,8 +782,10 @@ static int update_file_flags(struct merge_options *o,
else
mode = 0666;
fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
-   if (fd < 0)
-   die_errno(_("failed to open '%s'"), path);
+   if (fd < 0) {
+   ret = error_errno(_("failed to open '%s'"), 
path);
+   goto free_buf;
+   }
 
smudge_to_file = can_smudge_to_file(path);
 
@@ -792,8 +798,10 @@ static int update_file_flags(struct merge_options *o,
 * creation. */
smudge_to_file = 0;
fd = open(path, O_WRONLY | O_TRUNC | 
O_CREAT, mode);
-   if (fd < 0)
-   die_errno(_("failed to open 
'%s'"), path);
+   if (fd < 0) {
+   ret = error_errno(_("failed to 
open '%s'"), path);
+   goto free_buf;
+   }
}
else {
close(fd);
@@ -817,18 +825,18 @@ static int update_file_flags(struct merge_options *o,
safe_create_leading_directories_const(path);
unlink(path);
if (symlink(lnk, path))
-   die_errno(_("failed to symlink '%s'"), path);
+   ret = error_errno(_("failed to symlink '%s'"), 
path);
free(lnk);
} else
-   die(_("do not know what to do with %06o %s '%s'"),
+   ret = error(_("do not know what to do with %06o %s 

[PATCH v3 12/16] merge-recursive: flush output buffer before printing error messages

[Johannes Schindelin]

The data structure passed to the recursive merge machinery has a feature
where the caller can ask for the output to be buffered into a strbuf, by
setting the field 'buffer_output'.

Previously, we simply swallowed the buffered output when showing error
messages. With this patch, we show the output first, and only then print
the error message.

Currently, the only user of that buffering is merge_recursive() itself,
to avoid the progress output to interfere.

In the next patches, we will introduce a new buffer_output mode that
forces merge_recursive() to retain the output buffer for further
processing by the caller. If the caller asked for that, we will then
also write the error messages into the output buffer. This is necessary
to give the caller more control not only how to react in case of errors
but also control how/if to display the error messages.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 112 --
 1 file changed, 66 insertions(+), 46 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 7c9f22c..205ea04 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -23,6 +23,28 @@
 #include "dir.h"
 #include "submodule.h"
 
+static void flush_output(struct merge_options *o)
+{
+   if (o->obuf.len) {
+   fputs(o->obuf.buf, stdout);
+   strbuf_reset(>obuf);
+   }
+}
+
+static int err(struct merge_options *o, const char *err, ...)
+{
+   va_list params;
+
+   va_start(params, err);
+   flush_output(o);
+   strbuf_vaddf(>obuf, err, params);
+   error("%s", o->obuf.buf);
+   strbuf_reset(>obuf);
+   va_end(params);
+
+   return -1;
+}
+
 static struct tree *shift_tree_object(struct tree *one, struct tree *two,
  const char *subtree_shift)
 {
@@ -148,14 +170,6 @@ static int show(struct merge_options *o, int v)
return (!o->call_depth && o->verbosity >= v) || o->verbosity >= 5;
 }
 
-static void flush_output(struct merge_options *o)
-{
-   if (o->obuf.len) {
-   fputs(o->obuf.buf, stdout);
-   strbuf_reset(>obuf);
-   }
-}
-
 __attribute__((format (printf, 3, 4)))
 static void output(struct merge_options *o, int v, const char *fmt, ...)
 {
@@ -198,7 +212,8 @@ static void output_commit_title(struct merge_options *o, 
struct commit *commit)
}
 }
 
-static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
+static int add_cacheinfo(struct merge_options *o,
+   unsigned int mode, const struct object_id *oid,
const char *path, int stage, int refresh, int options)
 {
struct cache_entry *ce;
@@ -206,7 +221,7 @@ static int add_cacheinfo(unsigned int mode, const struct 
object_id *oid,
  (refresh ? (CE_MATCH_REFRESH |
  CE_MATCH_IGNORE_MISSING) : 0 ));
if (!ce)
-   return error(_("addinfo_cache failed for path '%s'"), path);
+   return err(o, _("addinfo_cache failed for path '%s'"), path);
return add_cache_entry(ce, options);
 }
 
@@ -267,7 +282,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 
if (!cache_tree_fully_valid(active_cache_tree) &&
cache_tree_update(_index, 0) < 0) {
-   error(_("error building trees"));
+   err(o, _("error building trees"));
return NULL;
}
 
@@ -535,7 +550,8 @@ static struct string_list *get_renames(struct merge_options 
*o,
return renames;
 }
 
-static int update_stages(const char *path, const struct diff_filespec *o,
+static int update_stages(struct merge_options *opt, const char *path,
+const struct diff_filespec *o,
 const struct diff_filespec *a,
 const struct diff_filespec *b)
 {
@@ -554,13 +570,13 @@ static int update_stages(const char *path, const struct 
diff_filespec *o,
if (remove_file_from_cache(path))
return -1;
if (o)
-   if (add_cacheinfo(o->mode, >oid, path, 1, 0, options))
+   if (add_cacheinfo(opt, o->mode, >oid, path, 1, 0, options))
return -1;
if (a)
-   if (add_cacheinfo(a->mode, >oid, path, 2, 0, options))
+   if (add_cacheinfo(opt, a->mode, >oid, path, 2, 0, options))
return -1;
if (b)
-   if (add_cacheinfo(b->mode, >oid, path, 3, 0, options))
+   if (add_cacheinfo(opt, b->mode, >oid, path, 3, 0, options))
return -1;
return 0;
 }
@@ -711,8 +727,8 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
if (status) {
if (status == SCLD_EXISTS)
/* something else exists */
-   return error(msg, 

[PATCH v3 04/16] merge-recursive: clarify code in was_tracked()

[Johannes Schindelin]

It can be puzzling to see that was_tracked() asks to get an index entry
by name, but does not take a negative return value for an answer.

The reason we have to do this is that cache_name_pos() only looks for
entries in stage 0, even if nobody asked for any stage in particular.

Let's rewrite the logic a little bit, to handle the easy case early: if
cache_name_pos() returned a non-negative position, we know it is a match,
and we do not even have to compare the name again (cache_name_pos() did
that for us already). We can say right away: yes, this file was tracked.

Only if there was no exact match do we need to look harder for any
matching entry in stage 2.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 30 ++
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index ea0df22..469741d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -658,23 +658,21 @@ static int was_tracked(const char *path)
 {
int pos = cache_name_pos(path, strlen(path));
 
-   if (pos < 0)
-   pos = -1 - pos;
-   while (pos < active_nr &&
-  !strcmp(path, active_cache[pos]->name)) {
-   /*
-* If stage #0, it is definitely tracked.
-* If it has stage #2 then it was tracked
-* before this merge started.  All other
-* cases the path was not tracked.
-*/
-   switch (ce_stage(active_cache[pos])) {
-   case 0:
-   case 2:
+   if (0 <= pos)
+   /* we have been tracking this path */
+   return 1;
+
+   /*
+* Look for an unmerged entry for the path,
+* specifically stage #2, which would indicate
+* that "our" side before the merge started
+* had the path tracked (and resulted in a conflict).
+*/
+   for (pos = -1 - pos;
+pos < active_nr && !strcmp(path, active_cache[pos]->name);
+pos++)
+   if (ce_stage(active_cache[pos]) == 2)
return 1;
-   }
-   pos++;
-   }
return 0;
 }
 
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 06/16] merge_recursive: abort properly upon errors

[Johannes Schindelin]

There are a couple of places where return values indicating errors
are ignored. Let's teach them manners.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 469741d..37c181a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1974,8 +1974,9 @@ int merge_recursive(struct merge_options *o,
saved_b2 = o->branch2;
o->branch1 = "Temporary merge branch 1";
o->branch2 = "Temporary merge branch 2";
-   merge_recursive(o, merged_common_ancestors, iter->item,
-   NULL, _common_ancestors);
+   if (merge_recursive(o, merged_common_ancestors, iter->item,
+   NULL, _common_ancestors) < 0)
+   return -1;
o->branch1 = saved_b1;
o->branch2 = saved_b2;
o->call_depth--;
@@ -1991,6 +1992,8 @@ int merge_recursive(struct merge_options *o,
o->ancestor = "merged common ancestors";
clean = merge_trees(o, h1->tree, h2->tree, 
merged_common_ancestors->tree,
);
+   if (clean < 0)
+   return clean;
 
if (o->call_depth) {
*result = make_virtual_commit(mrtree, "merged tree");
@@ -2047,6 +2050,9 @@ int merge_recursive_generic(struct merge_options *o,
hold_locked_index(lock, 1);
clean = merge_recursive(o, head_commit, next_commit, ca,
result);
+   if (clean < 0)
+   return clean;
+
if (active_cache_changed &&
write_locked_index(_index, lock, COMMIT_LOCK))
return error(_("Unable to write index."));
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 11/16] am -3: use merge_recursive() directly again

[Johannes Schindelin]

Last October, we had to change this code to run `git merge-recursive`
in a child process: git-am wants to print some helpful advice when the
merge failed, but the code in question was not prepared to return, it
die()d instead.

We are finally at a point when the code *is* prepared to return errors,
and can avoid the child process again.

This reverts commit c63d4b2 (am -3: do not let failed merge from
completing the error codepath, 2015-10-09), with the necessary changes
to adjust for the fact that Git's source code changed in the meantime
(such as: using OIDs instead of hashes in the recursive merge). While at
it, the same undesired gender bias is addressed in the same spirit as in
the separately submitted patch in $gmane/299009.

Note: the code now calls merge_recursive_generic() again. Unlike
merge_trees() and merge_recursive(), this function returns 0 upon success,
as most of Git's functions. Therefore, the error value -1 naturally is
handled correctly, and we do not have to take care of it specifically.

Signed-off-by: Johannes Schindelin 
---
 builtin/am.c | 63 ++--
 1 file changed, 23 insertions(+), 40 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index f9a724e..8dc4239 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -29,6 +29,7 @@
 #include "prompt.h"
 #include "mailinfo.h"
 #include "apply.h"
+#include "object.h"
 
 /**
  * Returns the length of the first line of msg.
@@ -1623,47 +1624,18 @@ static int build_fake_ancestor(const struct am_state 
*state, const char *index_f
 }
 
 /**
- * Do the three-way merge using fake ancestor, his tree constructed
- * from the fake ancestor and the postimage of the patch, and our
- * state.
- */
-static int run_fallback_merge_recursive(const struct am_state *state,
-   unsigned char *orig_tree,
-   unsigned char *our_tree,
-   unsigned char *his_tree)
-{
-   struct child_process cp = CHILD_PROCESS_INIT;
-   int status;
-
-   cp.git_cmd = 1;
-
-   argv_array_pushf(_array, "GITHEAD_%s=%.*s",
-sha1_to_hex(his_tree), linelen(state->msg), 
state->msg);
-   if (state->quiet)
-   argv_array_push(_array, "GIT_MERGE_VERBOSITY=0");
-
-   argv_array_push(, "merge-recursive");
-   argv_array_push(, sha1_to_hex(orig_tree));
-   argv_array_push(, "--");
-   argv_array_push(, sha1_to_hex(our_tree));
-   argv_array_push(, sha1_to_hex(his_tree));
-
-   status = run_command() ? (-1) : 0;
-   discard_cache();
-   read_cache();
-   return status;
-}
-
-/**
  * Attempt a threeway merge, using index_path as the temporary index.
  */
 static int fall_back_threeway(const struct am_state *state, const char 
*index_path)
 {
-   unsigned char orig_tree[GIT_SHA1_RAWSZ], his_tree[GIT_SHA1_RAWSZ],
- our_tree[GIT_SHA1_RAWSZ];
+   struct object_id orig_tree, her_tree, our_tree;
+   const struct object_id *bases[1] = { _tree };
+   struct merge_options o;
+   struct commit *result;
+   char *her_tree_name;
 
-   if (get_sha1("HEAD", our_tree) < 0)
-   hashcpy(our_tree, EMPTY_TREE_SHA1_BIN);
+   if (get_oid("HEAD", _tree) < 0)
+   hashcpy(our_tree.hash, EMPTY_TREE_SHA1_BIN);
 
if (build_fake_ancestor(state, index_path))
return error("could not build fake ancestor");
@@ -1671,7 +1643,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
discard_cache();
read_cache_from(index_path);
 
-   if (write_index_as_tree(orig_tree, _index, index_path, 0, NULL))
+   if (write_index_as_tree(orig_tree.hash, _index, index_path, 0, 
NULL))
return error(_("Repository lacks necessary blobs to fall back 
on 3-way merge."));
 
say(state, stdout, _("Using index info to reconstruct a base tree..."));
@@ -1687,7 +1659,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
init_revisions(_info, NULL);
rev_info.diffopt.output_format = DIFF_FORMAT_NAME_STATUS;
diff_opt_parse(_info.diffopt, _filter_str, 1, 
rev_info.prefix);
-   add_pending_sha1(_info, "HEAD", our_tree, 0);
+   add_pending_sha1(_info, "HEAD", our_tree.hash, 0);
diff_setup_done(_info.diffopt);
run_diff_index(_info, 1);
}
@@ -1696,7 +1668,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
return error(_("Did you hand edit your patch?\n"
"It does not apply to blobs recorded in its 
index."));
 
-   if (write_index_as_tree(his_tree, _index, index_path, 0, NULL))
+   if (write_index_as_tree(her_tree.hash, _index, index_path, 0, NULL))
return 

[PATCH v3 08/16] merge-recursive: allow write_tree_from_memory() to error out

[Johannes Schindelin]

It is possible that a tree cannot be written (think: disk full). We
will want to give the caller a chance to clean up instead of letting
the program die() in such a case.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index d9221ce..09f4e27 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1903,8 +1903,8 @@ int merge_trees(struct merge_options *o,
else
clean = 1;
 
-   if (o->call_depth)
-   *result = write_tree_from_memory(o);
+   if (o->call_depth && !(*result = write_tree_from_memory(o)))
+   return -1;
 
return clean;
 }
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 05/16] Prepare the builtins for a libified merge_recursive()

[Johannes Schindelin]

Previously, callers of merge_trees() or merge_recursive() expected that
code to die() with an error message. This used to be okay because we
called those commands from scripts, and had a chance to print out a
message in case the command failed fatally (read: with exit code 128).

As scripting incurs its own set of problems (portability, speed,
idiosynchracies of different shells, limited data structures leading to
inefficient code), we are converting more and more of these scripts into
builtins, using library functions directly.

We already tried to use merge_recursive() directly in the builtin
git-am, for example. Unfortunately, we had to roll it back temporarily
because some of the code in merge-recursive.c still deemed it okay to
call die(), when the builtin am code really wanted to print out a useful
advice after the merge failed fatally. In the next commits, we want to
fix that.

The code touched by this commit expected merge_trees() to die() with
some useful message when there is an error condition, but merge_trees()
is going to be improved by converting all die() calls to return error()
instead (i.e. return value -1 after printing out the message as before),
so that the caller can react more flexibly.

This is a step to prepare for the version of merge_trees() that no
longer dies,  even if we just imitate the previous behavior by calling
exit(128): this is what callers of e.g. `git merge` have come to expect.

Note that the callers of the sequencer (revert and cherry-pick) already
fail fast even for the return value -1; The only difference is that they
now get a chance to say " failed".

A caller of merge_trees() might want handle error messages themselves
(or even suppress them). As this patch is already complex enough, we
leave that change for a later patch.

Signed-off-by: Johannes Schindelin 
---
 builtin/checkout.c | 4 +++-
 builtin/merge.c| 2 ++
 sequencer.c| 4 
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 27c1a05..07dea3b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -567,8 +567,10 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
o.ancestor = old->name;
o.branch1 = new->name;
o.branch2 = "local";
-   merge_trees(, new->commit->tree, work,
+   ret = merge_trees(, new->commit->tree, work,
old->commit->tree, );
+   if (ret < 0)
+   exit(128);
ret = reset_tree(new->commit->tree, opts, 0,
 writeout_error);
if (ret)
diff --git a/builtin/merge.c b/builtin/merge.c
index 46b88ad..6837e15 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -682,6 +682,8 @@ static int try_merge_strategy(const char *strategy, struct 
commit_list *common,
hold_locked_index(, 1);
clean = merge_recursive(, head,
remoteheads->item, reversed, );
+   if (clean < 0)
+   exit(128);
if (active_cache_changed &&
write_locked_index(_index, , COMMIT_LOCK))
die (_("unable to write %s"), get_index_file());
diff --git a/sequencer.c b/sequencer.c
index cdfac82..286a435 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -293,6 +293,8 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
clean = merge_trees(,
head_tree,
next_tree, base_tree, );
+   if (clean < 0)
+   return clean;
 
if (active_cache_changed &&
write_locked_index(_index, _lock, COMMIT_LOCK))
@@ -559,6 +561,8 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
if (!opts->strategy || !strcmp(opts->strategy, "recursive") || 
opts->action == REPLAY_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
 head, , opts);
+   if (res < 0)
+   return res;
write_message(, git_path_merge_msg());
} else {
struct commit_list *common = NULL;
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 09/16] merge-recursive: handle return values indicating errors

[Johannes Schindelin]

We are about to libify the recursive merge machinery, where we only
die() in case of a bug or memory contention. To that end, we must heed
negative return values as indicating errors.

This requires our functions to be careful to pass through error
conditions in call chains, and for quite a few functions this means
that they have to return values to begin with.

The next step will be to convert the places where we currently die() to
return negative values (read: -1) instead.

Note that we ignore errors reported by make_room_for_path(), consistent
with the previous behavior (update_file_flags() used the return value of
make_room_for_path() only to indicate an early return, but not a fatal
error): if the error is really a fatal error, we will notice later; If
not, it was not that serious a problem to begin with. (Witnesses in
favor of this reasoning are t4151-am-abort and t7610-mergetool, which
would start failing if we stopped on errors reported by
make_room_for_path()).

Note: while this patch makes the code slightly less readable in
update_file_flags() (we introduce a new "goto free_buf;" instead of
an explicit "free(buf); return;"), it is a preparatory change for
the next patch where we will convert all of the die() calls in the same
function to go through the free_buf return path instead.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 199 +-
 1 file changed, 123 insertions(+), 76 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 09f4e27..89eb937 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -733,7 +733,7 @@ static int make_room_for_path(struct merge_options *o, 
const char *path)
return error(msg, path, _(": perhaps a D/F conflict?"));
 }
 
-static void update_file_flags(struct merge_options *o,
+static int update_file_flags(struct merge_options *o,
  const struct object_id *oid,
  unsigned mode,
  const char *path,
@@ -766,8 +766,7 @@ static void update_file_flags(struct merge_options *o,
 
if (make_room_for_path(o, path) < 0) {
update_wd = 0;
-   free(buf);
-   goto update_index;
+   goto free_buf;
}
if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {
int fd;
@@ -823,20 +822,22 @@ static void update_file_flags(struct merge_options *o,
} else
die(_("do not know what to do with %06o %s '%s'"),
mode, oid_to_hex(oid), path);
+ free_buf:
free(buf);
}
  update_index:
if (update_cache)
add_cacheinfo(mode, oid, path, 0, update_wd, 
ADD_CACHE_OK_TO_ADD);
+   return 0;
 }
 
-static void update_file(struct merge_options *o,
+static int update_file(struct merge_options *o,
int clean,
const struct object_id *oid,
unsigned mode,
const char *path)
 {
-   update_file_flags(o, oid, mode, path, o->call_depth || clean, 
!o->call_depth);
+   return update_file_flags(o, oid, mode, path, o->call_depth || clean, 
!o->call_depth);
 }
 
 /* Low level file merging, update and removal */
@@ -1034,7 +1035,7 @@ static int merge_file_one(struct merge_options *o,
return merge_file_1(o, , , , branch1, branch2, mfi);
 }
 
-static void handle_change_delete(struct merge_options *o,
+static int handle_change_delete(struct merge_options *o,
 const char *path,
 const struct object_id *o_oid, int o_mode,
 const struct object_id *a_oid, int a_mode,
@@ -1042,6 +1043,7 @@ static void handle_change_delete(struct merge_options *o,
 const char *change, const char *change_past)
 {
char *renamed = NULL;
+   int ret = 0;
if (dir_in_way(path, !o->call_depth)) {
renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
}
@@ -1052,21 +1054,22 @@ static void handle_change_delete(struct merge_options 
*o,
 * correct; since there is no true "middle point" between
 * them, simply reuse the base version for virtual merge base.
 */
-   remove_file_from_cache(path);
-   update_file(o, 0, o_oid, o_mode, renamed ? renamed : path);
+   ret = remove_file_from_cache(path);
+   if (!ret)
+   ret = update_file(o, 0, o_oid, o_mode, renamed ? 
renamed : path);
} else if (!a_oid) {
if (!renamed) {
output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
   "and %s in %s. Version %s of %s left in 

[PATCH v3 07/16] merge-recursive: avoid returning a wholesale struct

[Johannes Schindelin]

It is technically allowed, as per C89, for functions' return type to
be complete structs (i.e. *not* just pointers to structs).

However, it was just an oversight of this developer when converting
Python code to C code in 6d297f8 (Status update on merge-recursive in
C, 2006-07-08) which introduced such a return type.

Besides, by converting this construct to pass in the struct, we can now
start returning a value that can indicate errors in future patches. This
will help the current effort to libify merge-recursive.c.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 91 +--
 1 file changed, 48 insertions(+), 43 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 37c181a..d9221ce 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -910,47 +910,47 @@ static int merge_3way(struct merge_options *o,
return merge_status;
 }
 
-static struct merge_file_info merge_file_1(struct merge_options *o,
+static int merge_file_1(struct merge_options *o,
   const struct diff_filespec *one,
   const struct diff_filespec *a,
   const struct diff_filespec *b,
   const char *branch1,
-  const char *branch2)
+  const char *branch2,
+  struct merge_file_info *result)
 {
-   struct merge_file_info result;
-   result.merge = 0;
-   result.clean = 1;
+   result->merge = 0;
+   result->clean = 1;
 
if ((S_IFMT & a->mode) != (S_IFMT & b->mode)) {
-   result.clean = 0;
+   result->clean = 0;
if (S_ISREG(a->mode)) {
-   result.mode = a->mode;
-   oidcpy(, >oid);
+   result->mode = a->mode;
+   oidcpy(>oid, >oid);
} else {
-   result.mode = b->mode;
-   oidcpy(, >oid);
+   result->mode = b->mode;
+   oidcpy(>oid, >oid);
}
} else {
if (!oid_eq(>oid, >oid) && !oid_eq(>oid, >oid))
-   result.merge = 1;
+   result->merge = 1;
 
/*
 * Merge modes
 */
if (a->mode == b->mode || a->mode == one->mode)
-   result.mode = b->mode;
+   result->mode = b->mode;
else {
-   result.mode = a->mode;
+   result->mode = a->mode;
if (b->mode != one->mode) {
-   result.clean = 0;
-   result.merge = 1;
+   result->clean = 0;
+   result->merge = 1;
}
}
 
if (oid_eq(>oid, >oid) || oid_eq(>oid, >oid))
-   oidcpy(, >oid);
+   oidcpy(>oid, >oid);
else if (oid_eq(>oid, >oid))
-   oidcpy(, >oid);
+   oidcpy(>oid, >oid);
else if (S_ISREG(a->mode)) {
mmbuffer_t result_buf;
int merge_status;
@@ -962,64 +962,65 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
die(_("Failed to execute internal merge"));
 
if (write_sha1_file(result_buf.ptr, result_buf.size,
-   blob_type, result.oid.hash))
+   blob_type, result->oid.hash))
die(_("Unable to add %s to database"),
a->path);
 
free(result_buf.ptr);
-   result.clean = (merge_status == 0);
+   result->clean = (merge_status == 0);
} else if (S_ISGITLINK(a->mode)) {
-   result.clean = merge_submodule(result.oid.hash,
+   result->clean = merge_submodule(result->oid.hash,
   one->path,
   one->oid.hash,
   a->oid.hash,
   b->oid.hash,
   !o->call_depth);
} else if (S_ISLNK(a->mode)) {
-   oidcpy(, >oid);
+   oidcpy(>oid, >oid);
 
if (!oid_eq(>oid, >oid))
-   result.clean = 0;
+   

[PATCH v3 03/16] Avoid translating bug messages

[Johannes Schindelin]

While working on the patch series that avoids die()ing in recursive
merges, the issue came up that bug reports (i.e. die("BUG: ...")
constructs) should never be translated, as the target audience is the
Git developer community, not necessarily the current user, and hence
a translated message would make it *harder* to address the problem.

So let's stop translating the obvious ones. As it is really, really
outside the purview of this patch series to see whether there are more
die() statements that report bugs and are currently translated, that
task is left for another day and patch.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 05b9789..ea0df22 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -983,7 +983,7 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
if (!oid_eq(>oid, >oid))
result.clean = 0;
} else
-   die(_("BUG: unsupported object type in the tree"));
+   die("BUG: unsupported object type in the tree");
}
 
return result;
@@ -1827,7 +1827,7 @@ static int process_entry(struct merge_options *o,
 */
remove_file(o, 1, path, !a_mode);
} else
-   die(_("BUG: fatal merge failure, shouldn't happen."));
+   die("BUG: fatal merge failure, shouldn't happen.");
 
return clean_merge;
 }
@@ -1885,7 +1885,7 @@ int merge_trees(struct merge_options *o,
for (i = 0; i < entries->nr; i++) {
struct stage_data *e = entries->items[i].util;
if (!e->processed)
-   die(_("BUG: unprocessed path??? %s"),
+   die("BUG: unprocessed path??? %s",
entries->items[i].string);
}
 
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 14/16] merge-recursive: offer an option to retain the output in 'obuf'

[Johannes Schindelin]

Since 66a155b (Enable output buffering in merge-recursive., 2007-01-14),
we already accumulate the output in a buffer. The idea was to avoid
interfering with the progress output that goes to stderr, which is
unbuffered, when we write to stdout, which is buffered.

We extend that buffering to allow the caller to handle the output
(possibly suppressing it). This will help us when extending the
sequencer to do rebase -i's brunt work: it does not want the picks to
print anything by default but instead determine itself whether to print
the output or not.

Note that we also redirect the error messages into the output buffer
when the caller asked not to flush the output buffer, for two reasons:
1) to retain the correct output order, and 2) to allow the caller to
suppress *all* output.

Signed-off-by: Johannes Schindelin 
---
 merge-recursive.c | 17 +
 merge-recursive.h |  2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index ad5b961..10d3355 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -25,7 +25,7 @@
 
 static void flush_output(struct merge_options *o)
 {
-   if (o->obuf.len) {
+   if (o->buffer_output < 2 && o->obuf.len) {
fputs(o->obuf.buf, stdout);
strbuf_reset(>obuf);
}
@@ -36,10 +36,19 @@ static int err(struct merge_options *o, const char *err, 
...)
va_list params;
 
va_start(params, err);
-   flush_output(o);
+   if (o->buffer_output < 2)
+   flush_output(o);
+   else {
+   strbuf_complete(>obuf, '\n');
+   strbuf_addstr(>obuf, "error: ");
+   }
strbuf_vaddf(>obuf, err, params);
-   error("%s", o->obuf.buf);
-   strbuf_reset(>obuf);
+   if (o->buffer_output > 1)
+   strbuf_addch(>obuf, '\n');
+   else {
+   error("%s", o->obuf.buf);
+   strbuf_reset(>obuf);
+   }
va_end(params);
 
return -1;
diff --git a/merge-recursive.h b/merge-recursive.h
index d415724..340704c 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -13,7 +13,7 @@ struct merge_options {
MERGE_RECURSIVE_THEIRS
} recursive_variant;
const char *subtree_shift;
-   unsigned buffer_output : 1;
+   unsigned buffer_output : 2; /* 1: output at end, 2: keep buffered */
unsigned renormalize : 1;
long xdl_opts;
int verbosity;
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 01/16] Verify that `git pull --rebase` shows the helpful advice when failing

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 t/t5520-pull.sh | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 5d4880e..217b416 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -255,6 +255,36 @@ test_expect_success '--rebase' '
test new = "$(git show HEAD:file2)"
 '
 
+test_expect_success '--rebase with conflicts shows advice' '
+   test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
+   git checkout -b seq &&
+   printf "1\\n2\\n3\\n4\\n5\\n" >seq.txt &&
+   git add seq.txt &&
+   test_tick &&
+   git commit -m "Add seq.txt" &&
+   printf "6\\n" >>seq.txt &&
+   test_tick &&
+   git commit -m "Append to seq.txt" seq.txt &&
+   git checkout -b with-conflicts HEAD^ &&
+   printf "conflicting\\n" >>seq.txt &&
+   test_tick &&
+   git commit -m "Create conflict" seq.txt &&
+   test_must_fail git pull --rebase . seq 2>err >out &&
+   grep "When you have resolved this problem" out
+'
+test_expect_success 'failed --rebase shows advice' '
+   test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
+   git checkout -b diverging &&
+   test_commit attributes .gitattributes "* text=auto" attrs &&
+   sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" &&
+   git update-index --cacheinfo 0644 $sha1 file &&
+   git commit -m v1-with-cr &&
+   git checkout -f -b fails-to-rebase HEAD^ &&
+   test_commit v2-without-cr file "2" file2-lf &&
+   test_must_fail git pull --rebase . diverging 2>err >out &&
+   grep "When you have resolved this problem" out
+'
+
 test_expect_success '--rebase fails with multiple branches' '
git reset --hard before-rebase &&
test_must_fail git pull --rebase . copy master 2>err &&
-- 
2.9.0.278.g1caae67


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 00/16] Use merge_recursive() directly in the builtin am

[Johannes Schindelin]

This is the second iteration of the long-awaited re-roll of the attempt to
avoid spawning merge-recursive from the builtin am and use merge_recursive()
directly instead.

The *real* reason for the reroll is that I need a libified recursive
merge to accelerate the interactive rebase by teaching the sequencer to
do rebase -i's grunt work.

In this endeavor, we need to be extra careful to retain backwards
compatibility. The test script t6022-merge-rename.sh, for example, verifies
that `git pull` exits with status 128 in case of a fatal error. To that end,
we need to make sure that fatal errors are handled by existing (builtin)
users via exit(128) (or die(), which calls exit(128) at the end).  New users
(such as a builtin helper doing rebase -i's grunt work) may want to print
some helpful advice what happened and how to get out of this mess before
erroring out.

The changes relative to the second iteration of this patch series:

- the was_tracked() function was adjusted as per Junio's suggestions

- the "counter gender bias" patch was submitted separately, in
  $gmane/299009 (please note that the "am -3: use merge_recursive()
  directly again" patch is now slightly awkward as a consequence)

- this patch series is on top of 'pu', to address the conflicts with
  the 'jh/clean-smudge-annex' and the 'bc/cocci' branches

- please note that the interdiff does not show the full picture: I
  generated it relative to v2 rebased on top of pu (resolving many
  merge conflicts in the process that are hidden from the interdiff)

This patch series touches rather important code. Now that I addressed
concerns such as fixing translated bug reports, I would appreciate thorough
reviews with a focus on the critical parts of the code, those that could
result in regressions.


Johannes Schindelin (16):
  Verify that `git pull --rebase` shows the helpful advice when failing
  Report bugs consistently
  Avoid translating bug messages
  merge-recursive: clarify code in was_tracked()
  Prepare the builtins for a libified merge_recursive()
  merge_recursive: abort properly upon errors
  merge-recursive: avoid returning a wholesale struct
  merge-recursive: allow write_tree_from_memory() to error out
  merge-recursive: handle return values indicating errors
  merge-recursive: switch to returning errors instead of dying
  am -3: use merge_recursive() directly again
  merge-recursive: flush output buffer before printing error messages
  merge-recursive: write the commit title in one go
  merge-recursive: offer an option to retain the output in 'obuf'
  Ensure that the output buffer is released after calling merge_trees()
  merge-recursive: flush output buffer even when erroring out

 builtin/am.c   |  63 +++---
 builtin/checkout.c |   5 +-
 builtin/ls-files.c |   3 +-
 builtin/merge.c|   2 +
 builtin/update-index.c |   2 +-
 grep.c |   8 +-
 imap-send.c|   4 +-
 merge-recursive.c  | 508 +
 merge-recursive.h  |   2 +-
 sequencer.c|   5 +
 sha1_file.c|   4 +-
 t/t5520-pull.sh|  30 +++
 trailer.c  |   2 +-
 transport.c|   2 +-
 wt-status.c|   4 +-
 15 files changed, 382 insertions(+), 262 deletions(-)

Published-As: 
https://github.com/dscho/git/releases/tag/am-3-merge-recursive-direct-v3
Interdiff vs v2:

 diff --git a/builtin/am.c b/builtin/am.c
 index d626532..8dc4239 100644
 --- a/builtin/am.c
 +++ b/builtin/am.c
 @@ -29,6 +29,7 @@
  #include "prompt.h"
  #include "mailinfo.h"
  #include "apply.h"
 +#include "object.h"
  
  /**
   * Returns the length of the first line of msg.
 @@ -1627,15 +1628,14 @@ static int build_fake_ancestor(const struct am_state 
*state, const char *index_f
   */
  static int fall_back_threeway(const struct am_state *state, const char 
*index_path)
  {
 -  unsigned char orig_tree[GIT_SHA1_RAWSZ], her_tree[GIT_SHA1_RAWSZ],
 -our_tree[GIT_SHA1_RAWSZ];
 -  const unsigned char *bases[1] = {orig_tree};
 +  struct object_id orig_tree, her_tree, our_tree;
 +  const struct object_id *bases[1] = { _tree };
struct merge_options o;
struct commit *result;
char *her_tree_name;
  
 -  if (get_sha1("HEAD", our_tree) < 0)
 -  hashcpy(our_tree, EMPTY_TREE_SHA1_BIN);
 +  if (get_oid("HEAD", _tree) < 0)
 +  hashcpy(our_tree.hash, EMPTY_TREE_SHA1_BIN);
  
if (build_fake_ancestor(state, index_path))
return error("could not build fake ancestor");
 @@ -1643,7 +1643,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
discard_cache();
read_cache_from(index_path);
  
 -  if (write_index_as_tree(orig_tree, _index, index_path, 0, NULL))
 +  if (write_index_as_tree(orig_tree.hash, _index, index_path, 0, 
NULL))
return error(_("Repository lacks necessary blobs to fall back 
on 

[PATCH v3 02/16] Report bugs consistently

[Johannes Schindelin]

The vast majority of error messages in Git's source code which report a
bug use the convention to prefix the message with "BUG:".

As part of cleaning up merge-recursive to stop die()ing except in case of
detected bugs, let's just make the remainder of the bug reports consistent
with the de facto rule.

Signed-off-by: Johannes Schindelin 
---
 builtin/ls-files.c |  3 ++-
 builtin/update-index.c |  2 +-
 grep.c |  8 
 imap-send.c|  4 ++--
 merge-recursive.c  | 15 +++
 sha1_file.c|  4 ++--
 trailer.c  |  2 +-
 transport.c|  2 +-
 wt-status.c|  4 ++--
 9 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index f02e3d2..00ea91a 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -118,7 +118,8 @@ static void show_killed_files(struct dir_struct *dir)
 */
pos = cache_name_pos(ent->name, ent->len);
if (0 <= pos)
-   die("bug in show-killed-files");
+   die("BUG: killed-file %.*s not found",
+   ent->len, ent->name);
pos = -pos - 1;
while (pos < active_nr &&
   ce_stage(active_cache[pos]))
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 25af040..881a4b0 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1149,7 +1149,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
report(_("Untracked cache enabled for '%s'"), 
get_git_work_tree());
break;
default:
-   die("Bug: bad untracked_cache value: %d", untracked_cache);
+   die("BUG: bad untracked_cache value: %d", untracked_cache);
}
 
if (use_watchman > 0) {
diff --git a/grep.c b/grep.c
index 394c856..22cbb73 100644
--- a/grep.c
+++ b/grep.c
@@ -693,10 +693,10 @@ static struct grep_expr *prep_header_patterns(struct 
grep_opt *opt)
 
for (p = opt->header_list; p; p = p->next) {
if (p->token != GREP_PATTERN_HEAD)
-   die("bug: a non-header pattern in grep header list.");
+   die("BUG: a non-header pattern in grep header list.");
if (p->field < GREP_HEADER_FIELD_MIN ||
GREP_HEADER_FIELD_MAX <= p->field)
-   die("bug: unknown header field %d", p->field);
+   die("BUG: unknown header field %d", p->field);
compile_regexp(p, opt);
}
 
@@ -709,7 +709,7 @@ static struct grep_expr *prep_header_patterns(struct 
grep_opt *opt)
 
h = compile_pattern_atom();
if (!h || pp != p->next)
-   die("bug: malformed header expr");
+   die("BUG: malformed header expr");
if (!header_group[p->field]) {
header_group[p->field] = h;
continue;
@@ -1514,7 +1514,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
case GREP_BINARY_TEXT:
break;
default:
-   die("bug: unknown binary handling mode");
+   die("BUG: unknown binary handling mode");
}
}
 
diff --git a/imap-send.c b/imap-send.c
index db0fafe..67d67f8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -506,12 +506,12 @@ static char *next_arg(char **s)
 
 static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
 {
-   int ret;
+   int ret = -1;
va_list va;
 
va_start(va, fmt);
if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= 
(unsigned)blen)
-   die("Fatal: buffer too small. Please report a bug.");
+   die("BUG: buffer too small (%d < %d)", ret, blen);
va_end(va);
return ret;
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index 067f656..05b9789 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -259,7 +259,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
(int)ce_namelen(ce), ce->name);
}
-   die("Bug in merge-recursive.c");
+   die("BUG: unmerged index entries in merge-recursive.c");
}
 
if (!active_cache_tree)
@@ -982,9 +982,8 @@ static struct merge_file_info merge_file_1(struct 
merge_options *o,
 
if (!oid_eq(>oid, >oid))
result.clean = 0;
-   } else {
-   die(_("unsupported object 

Re: [PATCH] am: counteract gender bias

[Johannes Schindelin]

Hi Kuba,

On Thu, 7 Jul 2016, Jakub Narębski wrote:

> Instead of nebulous "fairness" (i.e., be unfair in other direction),
> [...]

There is nothing nebulous about it.

Ciao,
Dscho

Re: [PATCH] am: counteract gender bias

[Johannes Schindelin]

Hi Mike,

On Thu, 7 Jul 2016, Mike Hommey wrote:

> On Thu, Jul 07, 2016 at 01:47:19PM +0200, Johannes Schindelin wrote:
> > Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), i.e. for
> > almost 11 years already, we demonstrated our disrespect to the pioneers
> > of software development like Ada Lovelace, Grace Hopper and Margaret
> > Hamilton, by pretending that each and every software developer is male
> > ("his_tree"). It appears almost as if we weren't fully aware that the
> > first professional software developers were all female.
> > 
> > We know our field to have this unfortunate gender bias that has nothing
> > to do with qualification or biological reasons, and we are very sad
> > about the current gender imbalance of the Git developer community.
> > 
> > Let's start changing that by using the variable name "her_tree" for an
> > equal number of years out of fairness, and change to the gender neutral
> > "their_tree" after that.
> 
> You make it sound like the decision to use "his" was conscious and on
> purpose. I doubt that was the case, especially 11 years ago, when these
> issues weren't as publicized. Let's not attribute to malice on part of
> the people who wrote those lines what can be attributed to linguistics.

It was not my intention to imply that the original decision was conscious.

What with me being a non-native speaker, would you kindly suggest a commit
message that conveys the intention better?

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] am: counteract gender bias

[Jakub Narębski]

W dniu 2016-07-07 o 14:49, Mike Hommey pisze:
> On Thu, Jul 07, 2016 at 01:47:19PM +0200, Johannes Schindelin wrote:
>> Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), i.e. for
>> almost 11 years already, we demonstrated our disrespect to the pioneers
>> of software development like Ada Lovelace, Grace Hopper and Margaret
>> Hamilton, by pretending that each and every software developer is male
>> ("his_tree"). It appears almost as if we weren't fully aware that the
>> first professional software developers were all female.
>>
>> We know our field to have this unfortunate gender bias that has nothing
>> to do with qualification or biological reasons, and we are very sad
>> about the current gender imbalance of the Git developer community.
>>
>> Let's start changing that by using the variable name "her_tree" for an
>> equal number of years out of fairness, and change to the gender neutral
>> "their_tree" after that.
> 
> You make it sound like the decision to use "his" was conscious and on
> purpose. I doubt that was the case, especially 11 years ago, when these
> issues weren't as publicized. Let's not attribute to malice on part of
> the people who wrote those lines what can be attributed to linguistics.

Also, in all (?) other places we use "ours" and "theirs"; it looks like
git-am was a strange exception with "ours" and "his" (also, it was/is
inconsistent in using plural vs singular form).  Though perhaps it was
created before the terminology solidified...

As to why it was not noticed and not fixed: probably no-one worked
in this area, thus nobody noticed this (callee just don't care how the
parameter is named).

Instead of nebulous "fairness" (i.e., be unfair in other direction),
in my opinion it would be better to simply fix the issue, be consistent
and use common terminology.
-- 
Jakub Narębski

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] am: counteract gender bias

[Mike Hommey]

On Thu, Jul 07, 2016 at 01:47:19PM +0200, Johannes Schindelin wrote:
> Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), i.e. for
> almost 11 years already, we demonstrated our disrespect to the pioneers
> of software development like Ada Lovelace, Grace Hopper and Margaret
> Hamilton, by pretending that each and every software developer is male
> ("his_tree"). It appears almost as if we weren't fully aware that the
> first professional software developers were all female.
> 
> We know our field to have this unfortunate gender bias that has nothing
> to do with qualification or biological reasons, and we are very sad
> about the current gender imbalance of the Git developer community.
> 
> Let's start changing that by using the variable name "her_tree" for an
> equal number of years out of fairness, and change to the gender neutral
> "their_tree" after that.

You make it sound like the decision to use "his" was conscious and on
purpose. I doubt that was the case, especially 11 years ago, when these
issues weren't as publicized. Let's not attribute to malice on part of
the people who wrote those lines what can be attributed to linguistics.

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 21/21] mailmap: use main email address for dturner

[Johannes Schindelin]

Hi,

On Wed, 6 Jul 2016, Junio C Hamano wrote:

> David Turner  writes:
> 
> > Signed-off-by: David Turner 
> > ---
> >  .mailmap | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/.mailmap b/.mailmap
> > index e5b4126..edcae89 100644
> > --- a/.mailmap
> > +++ b/.mailmap
> > @@ -46,6 +46,7 @@ David D. Kilzer 
> >  David Kågedal 
> >  David Reiss  
> >  David S. Miller 
> > +David Turner  
> >  Deskin Miller 
> >  Dirk Süsserott 
> >  Eric Blake  
> 
> Let me take this separately and directly to the 'master', not as
> part of this series.
> 
> Not that I am rejecting the remainder of the series, though.

Speaking of which... Dave, I see that a lot of the patches in the series
still use the old mail address. Maybe change that?

Ciao,
Dscho

[PATCH] am: counteract gender bias

[Johannes Schindelin]

Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), i.e. for
almost 11 years already, we demonstrated our disrespect to the pioneers
of software development like Ada Lovelace, Grace Hopper and Margaret
Hamilton, by pretending that each and every software developer is male
("his_tree"). It appears almost as if we weren't fully aware that the
first professional software developers were all female.

We know our field to have this unfortunate gender bias that has nothing
to do with qualification or biological reasons, and we are very sad
about the current gender imbalance of the Git developer community.

Let's start changing that by using the variable name "her_tree" for an
equal number of years out of fairness, and change to the gender neutral
"their_tree" after that.

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/gender-bias-v1
 builtin/am.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index d5da5fe..2c7f3dd 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1584,14 +1584,14 @@ static int build_fake_ancestor(const struct am_state 
*state, const char *index_f
 }
 
 /**
- * Do the three-way merge using fake ancestor, his tree constructed
+ * Do the three-way merge using fake ancestor, her tree constructed
  * from the fake ancestor and the postimage of the patch, and our
  * state.
  */
 static int run_fallback_merge_recursive(const struct am_state *state,
unsigned char *orig_tree,
unsigned char *our_tree,
-   unsigned char *his_tree)
+   unsigned char *her_tree)
 {
struct child_process cp = CHILD_PROCESS_INIT;
int status;
@@ -1599,7 +1599,7 @@ static int run_fallback_merge_recursive(const struct 
am_state *state,
cp.git_cmd = 1;
 
argv_array_pushf(_array, "GITHEAD_%s=%.*s",
-sha1_to_hex(his_tree), linelen(state->msg), 
state->msg);
+sha1_to_hex(her_tree), linelen(state->msg), 
state->msg);
if (state->quiet)
argv_array_push(_array, "GIT_MERGE_VERBOSITY=0");
 
@@ -1607,7 +1607,7 @@ static int run_fallback_merge_recursive(const struct 
am_state *state,
argv_array_push(, sha1_to_hex(orig_tree));
argv_array_push(, "--");
argv_array_push(, sha1_to_hex(our_tree));
-   argv_array_push(, sha1_to_hex(his_tree));
+   argv_array_push(, sha1_to_hex(her_tree));
 
status = run_command() ? (-1) : 0;
discard_cache();
@@ -1620,7 +1620,7 @@ static int run_fallback_merge_recursive(const struct 
am_state *state,
  */
 static int fall_back_threeway(const struct am_state *state, const char 
*index_path)
 {
-   unsigned char orig_tree[GIT_SHA1_RAWSZ], his_tree[GIT_SHA1_RAWSZ],
+   unsigned char orig_tree[GIT_SHA1_RAWSZ], her_tree[GIT_SHA1_RAWSZ],
  our_tree[GIT_SHA1_RAWSZ];
 
if (get_sha1("HEAD", our_tree) < 0)
@@ -1657,7 +1657,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
return error(_("Did you hand edit your patch?\n"
"It does not apply to blobs recorded in its 
index."));
 
-   if (write_index_as_tree(his_tree, _index, index_path, 0, NULL))
+   if (write_index_as_tree(her_tree, _index, index_path, 0, NULL))
return error("could not write tree");
 
say(state, stdout, _("Falling back to patching base and 3-way 
merge..."));
@@ -1667,13 +1667,13 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
 
/*
 * This is not so wrong. Depending on which base we picked, orig_tree
-* may be wildly different from ours, but his_tree has the same set of
+* may be wildly different from ours, but her_tree has the same set of
 * wildly different changes in parts the patch did not touch, so
 * recursive ends up canceling them, saying that we reverted all those
 * changes.
 */
 
-   if (run_fallback_merge_recursive(state, orig_tree, our_tree, his_tree)) 
{
+   if (run_fallback_merge_recursive(state, orig_tree, our_tree, her_tree)) 
{
rerere(state->allow_rerere_autoupdate);
return error(_("Failed to merge in the changes."));
}
-- 
2.9.0.278.g1caae67

base-commit: 5c589a73de4394ad125a4effac227b3aec856fa1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/17] am: counteract gender bias

[Johannes Schindelin]

Hi Junio,

On Wed, 6 Jul 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), i.e. for
> > almost 11 years already,...
> > ...Let's start changing that by using the variable name "her_tree" for an
> > equal number of years out of fairness, and change to the gender neutral
> > "their_tree" after that.
> 
> I doubt this kind fo distraction is desirable in the middle of a
> seriously heavy series like this one.  As a standalone clean-up to
> turn these directly to "their" that everybody would agree on and can
> be merged down quickly to 'master' that does not have to keep the
> body of the main topic waiting for the dust to settle might be a
> better approach.
> 
> Unless you are trying to discourage the reviewers, that is ;-).

Funny. In other comments, I am asked to patch things that are truly
unrelated to the patch series' intent, and here I am asked to refrain from
cleaning up the code before I touch it.

I am really curious, though. Has it not been our practice to encourage
preparatory patches like white-space or const fixes as part of patch
series that touch a certain part of the code that needed fixing? I deem
this here patch to be much, much more important than a mere white-space or
const fix.

Since you asked so nicely, I will break out this patch from the patch
series, of course, but please note that it will now look as if I willfully
snuck in an unrelated change in the next patch, just because I was not
allowed to prepare the code properly.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/17] Report bugs consistently

[Johannes Schindelin]

Hi Duy,

On Wed, 6 Jul 2016, Duy Nguyen wrote:

> On Tue, Jul 5, 2016 at 1:23 PM, Johannes Schindelin
>  wrote:
> > The vast majority of error messages in Git's source code which report a
> > bug use the convention to prefix the message with "BUG:".
> >
> > As part of cleaning up merge-recursive to stop die()ing except in case of
> > detected bugs, let's just make the remainder of the bug reports consistent
> > with the de facto rule.
> 
> If you search 'die(_("bug:' in this patch,  you'll find 5 instances
> where _() should be gone too.

I should have known better. Vasco's i18n work already conflicts seriously
with what I have in this patch series.

This proves once again, beyond any doubt, that one should refrain from
introducing patches that have little to do with the purpose of the patch
series.

So as far as this here patch series goes, let's re-focus on the recursive
merge code again, okay?

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] merge-recursive: clarify code in was_tracked()

[Johannes Schindelin]

Hi Junio,

On Wed, 6 Jul 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > To understand why we're not done yet, the crucial point is *not* that the
> > return value encodes the insert position. The crucial point is that
> > despite asking for an index entry matching a specific name, we might not
> > find one, *even if there is one*.
> 
> I've been wondering why you keep saying "even though we didn't ask,
> we look for stage#0", and now I see why.  The cache_pos() interface
> *is* about finding the stage#0 entry for the given path.

Good that this is clarified now.

> [...]
> As you pointed out, we can return early without falling into the
> generic "we are still looking at the same path" codepath when we
> find thestage#0 entry, so I wouldn't mind doing something like the
> following.
> [...]

As this is essentially what I wrote with some minor touch-ups, I just
replaced my version with yours.

Will resend,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/17] Use merge_recursive() directly in the builtin am

[Johannes Schindelin]

Hi Junio,

On Wed, 6 Jul 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > This is the second iteration of the long-awaited re-roll of the attempt to
> > avoid spawning merge-recursive from the builtin am and use merge_recursive()
> > directly instead.
> 
> I wanted to queue this in 'pu', but an unfortunate series that
> changes the convert_to_git() infrastructure has serious conflicts
> with the changes in this series.  I am still unsure if these
> changes cannot be done without butchering the calling convention
> of what leads to convert_to_git(), but in the meantime, this cannot
> yet be merged to 'pu'.

There is nothing butchered there. The secret to the merge conflicts is the
sha1 -> oid conversion paired with Vasco's untranslating of BUG: reports
without upcasing the "bug:" prefix.

I will rebase to `pu` and re-send.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html