Re: gitconfig get out of sync with submodule entries on branch switch

[Benjamin Schindler]

Hi Brandon

I did try your suggestion, so basically:

git checkout branch
git submodule init
git submodule update

Unfortunately, I still have two entries in my git config this way. It 
seems that git submodule update only considers submodules listed in 
.gitmodules.


The background of my question is this - we have a jenkins farm which 
needs to switch branches continuously in an automated fashion and this 
needs to work even in when submodules are around. I did however, just 
now, find a reliable way to switch a branch, keeping the gitconfig in sync:

The basic workflow for switching a branch is:
git submodule deinit .
git checkout branch
git submodule init
git submodule update

Because the .git folder of the submodules are not within the submodule 
directories, this is, while still quite heavy-handed, reasonably fast 
and robust. At least it is better than deleting the entire repository 
every time a branch switch is issued.


Regards

Benjamin Schindler

On 30.01.2017 18:51, Brandon Williams wrote:

On 01/30, Benjamin Schindler wrote:

Hi

Consider the following usecase: I have the master branch where I
have a submodule A. I create a branch where I rename the submodule
to be in the directory B. After doing all of this, everything looks
good.
Now, I switch back to master. The first oddity is, that it fails to
remove the folder B because there are still files in there:

bschindler@metis ~/Projects/submodule_test (testbranch) $ git
checkout master
warning: unable to rmdir other_submodule: Directory not empty
Switched to branch 'master'

Git submodule deinit on B fails because the submodule is not known
to git anymore (after all, the folder B exists only in the other
branch). I can easily just remove the folder B from disk and
initialize the submodule A again, so all seems good.

However, what is not good is that the submodule b is still known in
.git/config. This is in particular a problem for us, because I know
a number of tools which use git config to retrieve the submodule
list. Is it therefore a bug that upon branch switch, the submodule
gets deregistered, but its entry in .git/config remains?

thanks a lot
Benjamin Schindler

P.s. I did not subscribe to the mailing list, please add me at least
do CC. Thanks

submodules and checkout don't really play nicely with each other at the
moment.  Stefan (cc'd) is currently working on a patch series to improve
the behavior of checkout with submodules.  Currently, if you want to
ensure you have a good working state after a checkout you should run
`git submodule update` to update all of the submoules.  As far as your
submodule still being listed in the config, that should be expected
given the scenario you described.

If I'm understanding you correctly, A and B are both the same submodule
just renamed on a different branch.  The moment you add a submoule to a
repository it is given a name which is fixed.  Typically this is the
path from the root of the repository.  The thing is, since you are able
to freely move a submodule, its path can change.  To account for this
there is the .gitmodules file which allows you to do a lookup from
submodule name to the path at which it exists (or vice versa).  The
submodules that are stored in .git/config are those which are
'initialize' or rather the submodules in which you are interested in and
will be updated by `git submodule update`.  So given your scenario you
should only have a single submodule in .git/config and the .gitmodules
file should have a single entry with a differing path for each branch.

Hopefully this gives you a bit more information to work with.  Since
Stefan has been working with this more recently than me he may have some
more input.

Git clonebundles

[Stefan Saasen]

Hi all,

Bitbucket recently added support for Mercurial’s clonebundle extension
(http://gregoryszorc.com/blog/2015/10/22/cloning-improvements-in-mercurial-3.6/).
Mercurial’s clone bundles allow the Mercurial client to seed a repository using
a bundle file instead of dynamically generating a bundle for the client.


Mercurial clonebundles?
~~~

With Mercurial clonebundles the high level clone sequence looks like this:

1. The command "hg clone URL"  attempts to clone the repository at URL.
2. If a bundle file exists for the repository, the existence of the file
`clonebundles.manifest` causes the server to advertise the `clonebundle`
capability (capabilities lookup is the first command the client issues).
3. In the above case the client then executes the command "clonebundles".
4. The manifest file will be returned.
5. The client then selects a bundle file to download from the list of URLs
advertised in the manifests file, to seed the repository.
6. To update the repository the last step involves fetching the latest changes.


Why is this useful?
~~~

The fact that clone bundles can be distributed as static files enables us to
use static file servers for bundle distribution. Users have also reported
latency improvements for clone operations of popular Mercurial repositories.
Additionally this significantly reduces the resource usage of clone operations,
as clone operations are reduced to simpler fetches to resolve the delta between
the current repository and the downloaded bundle state.


clonebundles for git?
~

We recently looked into how this concept could be translated to git. This is
not a new idea and has been discussed before (more on that later) but our
success with the Mercurial clonebundle rollout prompted us to revisit this
topic.

We believe that bringing a similar concept to git could have the following
benefits:

* Improved clone times for users that clone large git repositories, especially
  if bundle file distribution leverages global CDNs.
* Improved scalability of git for managing large popular repositories.
  Offloading a significant portion of the clone resource usage to CDNs or static
  file hosts.


Our current proof-of-concept to explore this space, closely follows
the approach from Mercurial outlined above.

* An `/info/bundle` path returns a bundle manifest (over HTTP)
* The bundle manifest contains a simple list of URLs with some additional meta
  data that allows the client to select a suitable bundle download URL
* The bundle download URL points to a bundle file generated using `git bundle
  create` including all the relevant refs as a self contained repository seed.
* The client probes the target URL with a `GET` request to $URL/info/bundle and
  downloads the bundle file if present.
* The repository will be created based on the downloaded bundle (downloading a
  static file allows resumable downloads or parallel downloads of chunks if the
  file/web server supports range requests).
* A `git fetch` and the appropriate checkout then updates the "cloned"
  repository to match the latest upstream state.

The proof-of-concept was built as an external binary `git-clone2` that
mimics the behaviour of the `git clone` command, so unfortunately I
can't provide any patches to git to demonstrate the behaviour.


Ultimately our proof-of-concept is built around a few core ideas:

* Re-use the existing bundle format as a single-file, self-contained
repository representation.
* Introduce a bundle manifest (accessible at `$URL/info/bundle`) that allows
  the client to resolve a suitable bundle download URL.
* Teach the `git clone` command to accept and prefer seeding a repository using
  a static bundle file that is advertised in a bundle manifest.
* Re-use as much as possible of the existing commands and in particular the
  `git bundle` machinery to seed the repository and to create the static bundle
  file.
* We accept additional storage requirements for the bundle files in addition to
  the actual repository content in pack-files or loose objects.
Hosting providers
  or system administrators are free to decide how many bundles to advertise and
  how frequently the bundles are updated.
* It targets the "seed from a bundle file" use case, with resumable clones just
  being a potential side-effect.


Some of the problems that need to be solved with an approach like this are:

* Bundle advertisement/bundle negotiation: We considered advertising a
  new capability "clonebundle" as part of the rev advertisement
capabilities list.
  This would allow clients that support clonebundles to abort the clone attempt
  and resolve a suitable bundle URL from a bundle manifest at `$URL/info/bundle`
  instead. For HTTP this would amount to an early termination when
retrieving the
  ref-advertisement.
  Note: We didn't pursue this for our proof-of-concept so we didn't
explore whether
  this is feasible.
* Uniform approach for the supported transports: Our 

Re: [PATCH 1/4] git-prompt.sh: add submodule indicator

[Junio C Hamano]

Benjamin Fuchs  writes:

> In [2/4] I got rid of the loop by feedback of Gábor.
> Sorry if my patch wasn't well formed.

While it might be the way other development communities work, in the
Git development community, we do not work that way when presenting
your second and subsequent attempt to the community.

Having the initial draft from the original developers that records
the bugs and misdesigns in an earlier parts of a series and separate
patches that record how the problems were fixed to arrive at the
final shape of the codebase might be interesting to the original
developers, and they may even find such a history valuable, but in
the context of the history that will be recorded in the official
tree of the project for eternity, that just adds useless noise.

Instead of keeping the original, in which problems were pointed out,
and adding later commits to correct them incrementally, a patch is
"rerolled".  That is, you are expected to learn from the review
comments and pretend as if you did the work from scratch and you
already possessed the wisdom lent by the reviewers when you started
your work.  In the "rerolled" patches you send, you pretend as if
you worked without making mistakes you made in the earlier rounds at
all, producing (more) perfect patches from the beginning.  

In reality, you may locally be using Git tools like rebase,
cherry-pick and "add -p" while preparing these "rerolled" rounds of
patches, but the name of the game is to hide that effort from the
public and pretend to be a perfect human, recording the result of
exercising your best ability in the official history ;-).

So this is OK:

0/3: I want to improve X, and for that I identified that I need
A, B and C done.  A or B alone is already an improvement, but A
and B together makes it even more useful, and implementation of
C requires patches to do A and B.

1/3: do A
2/3: do B
3/3: do C, building on A and B

This is not:

0/3: I want to improve X, and for that I need to do C.
1/3: I couldn't do C, and I did A instead.
2/3: A was totally useless. I fix it to do B.
3/3: B is not very useful, either. I fix it to do C.

Re: difflame

[Edmundo Carmona Antoranz]

Maybe a little work on blame to get the actual revision where some
lines were "deleted"?

Something like git blame --blame-deletion that could be based on --reverse?

On Mon, Jan 30, 2017 at 7:35 PM, Edmundo Carmona Antoranz
 wrote:
> I'm thinking of something like this:
>
> Say I just discovered a problem in a file I want to see who worked
> on it since some revision that I know is working fine (or even
> something as generic as HEAD~100..). It could be a number of people
> with different revisions. I would diff to see what changed and
> blame the added lines (blame reverse to try to get as close as
> possible with a single command in case I want to see what happened
> with something that was deleted). If I could get blame information of
> added/deleted lines in a single run, that would help a lot.
>
> Lo and behold: difflame.
>
>
>
> On Mon, Jan 30, 2017 at 5:26 PM, Jeff King  wrote:
>> On Mon, Jan 30, 2017 at 01:08:41PM -0800, Junio C Hamano wrote:
>>
>>> Jeff King  writes:
>>>
>>> > On Tue, Jan 17, 2017 at 11:24:02PM -0600, Edmundo Carmona Antoranz wrote:
>>> >
>>> >> For a very long time I had wanted to get the output of diff to include
>>> >> blame information as well (to see when something was added/removed).
>>> >
>>> > This is something I've wanted, too. The trickiest part, though, is
>>> > blaming deletions, because git-blame only tracks the origin of content,
>>> > not the origin of a change.
>>>
>>> Hmph, this is a comment without looking at what difflame does
>>> internally, so you can ignore me if I am misunderstood what problem
>>> you are pointing out, but I am not sure how "tracks the origin of
>>> content" could be a problem.
>>>
>>> If output from "git show" says this:
>>>
>>>   --- a/file
>>>   +++ b/file
>>>   @@ -1,5 +1,6 @@
>>>a
>>>b
>>>   -c
>>>   +C
>>>   +D
>>>d
>>>e
>>>
>>> in order to annotate lines 'a', 'b', 'd', and 'e' for their origin,
>>> you would run 'blame' on the commit the above output was taken from
>>> (or its parent---they are in the context so either would be OK).
>>>
>>> You know where 'C' and 'D' came from already.  It's the commit you
>>> are feeding "git show".
>>
>> I think the point (or at least as I understand it) is that the diff is
>> not "git show" for a particular commit. It could be part of a much
>> larger diff that covers many commits.
>>
>> As a non-hypothetical instance, I have a fork of git.git that has
>> various enhancements. I want to feed those enhancements upstream. I need
>> to know which commits should be cherry-picked to get those various
>> enhancements.
>>
>> Looking at "log origin..fork" tells me too many commits, because it
>> includes ones which aren't useful anymore. Either because they already
>> went upstream, or because they were cherry-picked to the fork and their
>> upstream counterparts merged (or even equivalent commits made upstream
>> that obsoleted the features).
>>
>> Looking at "git diff origin fork" tells me what the actual differences
>> are, but it doesn't show me which commits are responsible for them.
>>
>> I can "git blame" each individual line of the diff (starting with "fork"
>> as the tip), but that doesn't work for lines that no longer exist (i.e.,
>> when the interesting change is a deletion).
>>
>>> In order to run blame to find where 'c' came from, you need to start
>>> at the _parent_ of the commit the above output came from, and the
>>> hunk header shows which line range to find the final 'c'.
>>
>> So perhaps that explains my comment more. "blame" is not good for
>> finding which commit took away a line. I've tried using "blame
>> --reverse", but it shows you the parent of the commit you are looking
>> for, which is slightly annoying. :)
>>
>> "git log -S" is probably a better tool for finding that.
>>
>> -Peff

Re: [PATCH] blame: draft of line format

[Edmundo Carmona Antoranz]

I'm basing in on the "pretty API" so that we don't have to reinvent
the wheel. I have already noticed that many of the formatting options
available for pretty are not working... I'm sure it would require some
work setting up the call to pretty api but the basic is laid out
there.

Let me know of your thoughts.

[PATCH] blame: draft of line format

[Edmundo Carmona Antoranz]

---
 builtin/blame.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/builtin/blame.c b/builtin/blame.c
index 126b8c9e5..89c1a862d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -52,6 +52,7 @@ static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
 static int show_progress;
+static char *format_line;
 
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -1931,6 +1932,19 @@ static void emit_porcelain(struct scoreboard *sb, struct 
blame_entry *ent,
putchar('\n');
 }
 
+static void pretty_info(char* revid, struct blame_entry *ent, struct strbuf 
*rev_buffer)
+{
+   struct pretty_print_context ctx = {0};
+   struct rev_info rev;
+
+   struct strbuf format = STRBUF_INIT;
+   strbuf_addstr(, format_line);
+   ctx.fmt = CMIT_FMT_USERFORMAT;
+   get_commit_format(format.buf, );
+   pretty_print_commit(, ent->suspect->commit, rev_buffer);
+   strbuf_release();
+}
+
 static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
 {
int cnt;
@@ -1939,11 +1953,15 @@ static void emit_other(struct scoreboard *sb, struct 
blame_entry *ent, int opt)
struct commit_info ci;
char hex[GIT_SHA1_HEXSZ + 1];
int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+   struct strbuf line_revision_buf = STRBUF_INIT;
 
get_commit_info(suspect->commit, , 1);
sha1_to_hex_r(hex, suspect->commit->object.oid.hash);
 
cp = nth_line(sb, ent->lno);
+
+   if (format_line)
+   pretty_info(hex, ent, _revision_buf);
for (cnt = 0; cnt < ent->num_lines; cnt++) {
char ch;
int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : 
abbrev;
@@ -1968,6 +1986,10 @@ static void emit_other(struct scoreboard *sb, struct 
blame_entry *ent, int opt)
   format_time(ci.author_time, ci.author_tz.buf,
   show_raw_time),
   ent->lno + 1 + cnt);
+   } else if (format_line) {
+   printf("%s", line_revision_buf.buf);
+   printf(" %*d) ",
+  max_digits, ent->lno + 1 + cnt);
} else {
if (opt & OUTPUT_SHOW_SCORE)
printf(" %*d %02d",
@@ -2007,6 +2029,7 @@ static void emit_other(struct scoreboard *sb, struct 
blame_entry *ent, int opt)
if (sb->final_buf_size && cp[-1] != '\n')
putchar('\n');
 
+   strbuf_release(_revision_buf);
commit_info_destroy();
 }
 
@@ -2605,6 +2628,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
 
OPT_BIT(0, "minimal", _opts, N_("Spend extra cycles to find 
better match"), XDF_NEED_MINIMAL),
OPT_STRING('S', NULL, _file, N_("file"), N_("Use revisions 
from  instead of calling git-rev-list")),
+   OPT_STRING(0, "format-line", _line, N_("format-line"), 
N_("Use pretty format for revisions")),
OPT_STRING(0, "contents", _from, N_("file"), N_("Use 
's contents as the final image")),
{ OPTION_CALLBACK, 'C', NULL, , N_("score"), N_("Find line 
copies within and across files"), PARSE_OPT_OPTARG, blame_copy_callback },
{ OPTION_CALLBACK, 'M', NULL, , N_("score"), N_("Find line 
movements within and across files"), PARSE_OPT_OPTARG, blame_move_callback },
-- 
2.11.0.rc1

Re: difflame

[Edmundo Carmona Antoranz]

I'm thinking of something like this:

Say I just discovered a problem in a file I want to see who worked
on it since some revision that I know is working fine (or even
something as generic as HEAD~100..). It could be a number of people
with different revisions. I would diff to see what changed and
blame the added lines (blame reverse to try to get as close as
possible with a single command in case I want to see what happened
with something that was deleted). If I could get blame information of
added/deleted lines in a single run, that would help a lot.

Lo and behold: difflame.



On Mon, Jan 30, 2017 at 5:26 PM, Jeff King  wrote:
> On Mon, Jan 30, 2017 at 01:08:41PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>>
>> > On Tue, Jan 17, 2017 at 11:24:02PM -0600, Edmundo Carmona Antoranz wrote:
>> >
>> >> For a very long time I had wanted to get the output of diff to include
>> >> blame information as well (to see when something was added/removed).
>> >
>> > This is something I've wanted, too. The trickiest part, though, is
>> > blaming deletions, because git-blame only tracks the origin of content,
>> > not the origin of a change.
>>
>> Hmph, this is a comment without looking at what difflame does
>> internally, so you can ignore me if I am misunderstood what problem
>> you are pointing out, but I am not sure how "tracks the origin of
>> content" could be a problem.
>>
>> If output from "git show" says this:
>>
>>   --- a/file
>>   +++ b/file
>>   @@ -1,5 +1,6 @@
>>a
>>b
>>   -c
>>   +C
>>   +D
>>d
>>e
>>
>> in order to annotate lines 'a', 'b', 'd', and 'e' for their origin,
>> you would run 'blame' on the commit the above output was taken from
>> (or its parent---they are in the context so either would be OK).
>>
>> You know where 'C' and 'D' came from already.  It's the commit you
>> are feeding "git show".
>
> I think the point (or at least as I understand it) is that the diff is
> not "git show" for a particular commit. It could be part of a much
> larger diff that covers many commits.
>
> As a non-hypothetical instance, I have a fork of git.git that has
> various enhancements. I want to feed those enhancements upstream. I need
> to know which commits should be cherry-picked to get those various
> enhancements.
>
> Looking at "log origin..fork" tells me too many commits, because it
> includes ones which aren't useful anymore. Either because they already
> went upstream, or because they were cherry-picked to the fork and their
> upstream counterparts merged (or even equivalent commits made upstream
> that obsoleted the features).
>
> Looking at "git diff origin fork" tells me what the actual differences
> are, but it doesn't show me which commits are responsible for them.
>
> I can "git blame" each individual line of the diff (starting with "fork"
> as the tip), but that doesn't work for lines that no longer exist (i.e.,
> when the interesting change is a deletion).
>
>> In order to run blame to find where 'c' came from, you need to start
>> at the _parent_ of the commit the above output came from, and the
>> hunk header shows which line range to find the final 'c'.
>
> So perhaps that explains my comment more. "blame" is not good for
> finding which commit took away a line. I've tried using "blame
> --reverse", but it shows you the parent of the commit you are looking
> for, which is slightly annoying. :)
>
> "git log -S" is probably a better tool for finding that.
>
> -Peff

Re: [RFC] Proof of concept: Support multiple authors

[Cornelius Schumacher]

On Monday 30 January 2017 12:48:52 Junio C Hamano wrote:
> 
>   https://public-inbox.org/git/?q=gmane:83880
>   https://public-inbox.org/git/?q=gmane:146223
>   https://public-inbox.org/git/?q=gmane:146886

Thanks for putting the links together. That's very useful as a reference.

> The older discussions already cited the cost to update both in-tree
> and out-of-tree tools not to barf when they see such a commit object
> and one of the reason why we would not want to do this, and Ted Ts'o
> gave us another excellent reason why not to do multiple author
> header lines in a commit object header, i.e. "How often is that all
> of the authors are completely equal?"

Just to give a bit of context: In the pair programming environment where I 
work we usually use non-personalized workstations and switch the keyboard 
between the two people working together quite frequently, sometimes every few 
minutes, or even within writing a commit message. There the person pressing 
the return button on the commit really does not have a special role. In this 
style of working I think it feels like the fairest and most practical 
assumption to treat all authors as equal.

> My advice to those who want to record credit to multiple authors is
> to treat the commit author line as recording the primary contact
> when there is a question on the commit and nothing else.  Anything
> with richer semantics is better done in the trailer by enriching the
> support of trailer lines with interpret-trailers framework.

Thanks for the advice. I think I will explore this direction a little bit 
further and see how handling a situation of multiple authors could be better 
done in this framework.

-- 
Cornelius Schumacher 

Re: [ANNOUNCE] Git Merge Contributor Summit topic planning

[Jeff King]

On Tue, Jan 31, 2017 at 01:48:05AM +0100, Jeff King wrote:

> The list of topics is totally open. If you're coming and have something
> you'd like to present or discuss, then propose it here. If you're _not_
> coming, you may still chime in with input on topics, but please don't
> suggest a topic unless somebody who is there will agree to lead the
> discussion.

Here are the two topics I plan on bringing:

  - Git / Software Freedom Conservancy yearly report. I'll plan to give
a rundown of the past year's activities and financials, along with
some open questions that could benefit from community input.

  - The git-scm.com website: who runs that thing, anyway? An overview
of the site, how it's managed, and what it needs.

I plan to send out detailed emails on both topics to the list on
Wednesday, and then follow-up with a summary of any useful in-person
discussion (since obviously not everybody will be at the summit).

I'd encourage anybody with a topic to present to consider doing
something similar.

-Peff

[ANNOUNCE] Git Merge Contributor Summit topic planning

[Jeff King]

The Contributor Summit is only a few days away; I'd like to work out a
few bits of logistics ahead of time.

We're up to 26 attendees. The room layout will probably be three big
round-tables, with a central projector. We should be able to have
everybody pay attention to a single speaker, or break into 3 separate
conversations.

The list of topics is totally open. If you're coming and have something
you'd like to present or discuss, then propose it here. If you're _not_
coming, you may still chime in with input on topics, but please don't
suggest a topic unless somebody who is there will agree to lead the
discussion.

We'll write the final list on a whiteboard on Thursday morning, vote on
what looks good, and then work our way down the list.  Topics don't
_have_ to be proposed here ahead of time, but I'd encourage people to do
so as it leaves time for others to consider them and possibly do any
background thinking or research.

The rough schedule is:

  0830 to 0930 - registration, breakfast, milling about and socializing;
 be aware that Git Merge Workshop attendees will be
 doing the same things in the same space, so show up
 with enough time to navigate a bit of a crowd.

  0930 to 1215 - we retire to our Fortress of Solitude to talk about
 Very Important Git Things

  1215 to 1330 - lunch

  1330 to 1500 - Very Important Git Things, part deux. The end time
 isn't a hard deadline, so we can go as late as 1600 if
 the discussion keeps up.

There's no organized dinner planned. At our size, I think it's probably
most productive to let people form small groups for dinner if they want
to. But if somebody is really interested in trying to do a big group
reservation, they are welcome to try to organize it.

-Peff

Re: [PATCH] Completion: Add support for --submodule=diff

[Jacob Keller]

On Sun, Dec 4, 2016 at 6:41 AM,   wrote:
> From: Peter Law 
>
> Teach git-completion.bash about the 'diff' option to 'git diff
> --submodule=', which was added in Git 2.11.
>
> Signed-off-by: Peter Law 
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 21016bf8d..ab11e7371 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1206,7 +1206,7 @@ _git_describe ()
>
>  __git_diff_algorithms="myers minimal patience histogram"
>
> -__git_diff_submodule_formats="log short"
> +__git_diff_submodule_formats="diff log short"
>
>  __git_diff_common_options="--stat --numstat --shortstat --summary
> --patch-with-stat --name-only --name-status --color
> --
> 2.11.0
>

Yep, this looks good to me.

Thanks,
Jake

Re: [PATCH 1/4] git-prompt.sh: add submodule indicator

[Benjamin Fuchs]

Hi Junio,
thanks for your reply. Some of your suggestions are covered by my rework 
in Patch [2/4].

In [2/4] I got rid of the loop by feedback of Gábor.
Sorry if my patch wasn't well formed.

On 31.01.2017 00:48, Junio C Hamano wrote:

Benjamin Fuchs  writes:


I expirienced that working with submodules can be confusing. This indicator
will make you notice very easy when you switch into a submodule.

I am not quite sure what the above wants to say.  If you work on two
projects, A and B, then having something that quickly reminds you
which one you are in is beneficial and people often do so by having
the current directory name in the prompt.

The log message needs to explain why working on a submodule C of a
project A is any more special, i.e. why are the existing ways the
users are using to remind them between A and B cannot be used for C.
A submodule can be anywhere in your parent git repository. And while 
walking through the
parent repository it is sometime a good reminder to know when to just 
entered a submodule.
Because now all git command will work on the submodule. I guess this 
indicator is a good way

to show that.
And with patch [2/4] (and fixed it with [3/4]) I also introduced the 
"dirty" indicator for the submodule, which can
show you, that your current checked out state in the submodule is not 
the one committed

to the parent.




diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 97eacd7..4c82e7f 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -93,6 +93,10 @@
  # directory is set up to be ignored by git, then set
  # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
  # repository level by setting bash.hideIfPwdIgnored to "false".
+#
+# If you would like __git_ps1 to indicate that you are in a submodule,
+# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
+# the branch name.
  
  # check whether printf supports -v

  __git_printf_supports_v=
@@ -284,6 +288,32 @@ __git_eread ()
test -r "$f" && read "$@" <"$f"
  }
  
+# __git_is_submodule

+# Based on:
+# 
http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
+__git_is_submodule ()

It seems like this function is checking if you are "IN" submodule,
not "IS" submodule.  A misnamed function?

Reworked in Patch [2/4]

+{
+   local git_dir parent_git module_name path
+   # Find the root of this git repo, then check if its parent dir is also 
a repo
+   git_dir="$(git rev-parse --show-toplevel)"
+   module_name="$(basename "$git_dir")"

This does not give "module_name" in the sense the word is used in
the submodule subsystem.  If this variable is useful, call that
with "path" in its name (I do not think it alone is useful at all).

Reworked in Patch [2/4]



+   parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> 
/dev/null)"
+   if [[ -n $parent_git ]]; then
+   # List all the submodule paths for the parent repo
+   while read path
+   do
+   if [[ "$path" != "$module_name" ]]; then continue; fi
+   if [[ -d "$git_dir/../$path" ]];then return 0; fi
+   done < <(cd $parent_git && git submodule --quiet foreach 'echo 
$path' 2> /dev/null)

Instead of doing this "loop over submodules and just get true or
false", it may make a lot more sense to find the module name and
report that.  That would allow you to have the actual submodule
name, not a generic "sub:" which does not help the users to tell
which one of 47 submodules they have in their top-level project
they are currently in.

If your projects are layed out like so

/home/bf/projects/A/
/home/bf/projects/A/lib/B/ -- submodule
/home/bf/projects/A/Doc/ -- another submodule
/home/bf/projects/A/Doc/technical -- a subdirectory of Doc submodule

and you are in /home/bf/projects/A/Doc/technical/ subdirectory, your
$git_dir variable (which is grossly misnamed, too; call it "top"
perhaps?) would be "/home/bf/projects/A/Doc" and then your
$parent_git variable (again, that is misnamed; call it
"parent_top"?) would be "/home/bf/projects/A".  The submodule path
to the submodule you are currently in is "Doc" (you learn it as the
difference between these two).

You can ask the top-level project the name of the submodule that is
currently at "Doc" with "submodule--helper name".  Unless the project
has moved it since it first added the submodule, the module name may
also be "Doc", but if it were moved, it may be different.

And that "module name" is a more useful thing than a hardcoded
string "sub:" to use in the prompt, I would think.

Reworked in Patch [2/4].

Re: [PATCH 1/4] git-prompt.sh: add submodule indicator

[Junio C Hamano]

Benjamin Fuchs  writes:

> I expirienced that working with submodules can be confusing. This indicator
> will make you notice very easy when you switch into a submodule.

I am not quite sure what the above wants to say.  If you work on two
projects, A and B, then having something that quickly reminds you
which one you are in is beneficial and people often do so by having
the current directory name in the prompt.  

The log message needs to explain why working on a submodule C of a
project A is any more special, i.e. why are the existing ways the
users are using to remind them between A and B cannot be used for C.

> diff --git a/contrib/completion/git-prompt.sh 
> b/contrib/completion/git-prompt.sh
> index 97eacd7..4c82e7f 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -93,6 +93,10 @@
>  # directory is set up to be ignored by git, then set
>  # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
>  # repository level by setting bash.hideIfPwdIgnored to "false".
> +#
> +# If you would like __git_ps1 to indicate that you are in a submodule,
> +# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
> +# the branch name.
>  
>  # check whether printf supports -v
>  __git_printf_supports_v=
> @@ -284,6 +288,32 @@ __git_eread ()
>   test -r "$f" && read "$@" <"$f"
>  }
>  
> +# __git_is_submodule
> +# Based on:
> +# 
> http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
> +__git_is_submodule ()

It seems like this function is checking if you are "IN" submodule,
not "IS" submodule.  A misnamed function?

> +{
> + local git_dir parent_git module_name path
> + # Find the root of this git repo, then check if its parent dir is also 
> a repo
> + git_dir="$(git rev-parse --show-toplevel)"
> + module_name="$(basename "$git_dir")"

This does not give "module_name" in the sense the word is used in
the submodule subsystem.  If this variable is useful, call that
with "path" in its name (I do not think it alone is useful at all).

> + parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> 
> /dev/null)"
> + if [[ -n $parent_git ]]; then
> + # List all the submodule paths for the parent repo
> + while read path
> + do
> + if [[ "$path" != "$module_name" ]]; then continue; fi
> + if [[ -d "$git_dir/../$path" ]];then return 0; fi
> + done < <(cd $parent_git && git submodule --quiet foreach 'echo 
> $path' 2> /dev/null)

Instead of doing this "loop over submodules and just get true or
false", it may make a lot more sense to find the module name and
report that.  That would allow you to have the actual submodule
name, not a generic "sub:" which does not help the users to tell
which one of 47 submodules they have in their top-level project
they are currently in.

If your projects are layed out like so

/home/bf/projects/A/
/home/bf/projects/A/lib/B/ -- submodule
/home/bf/projects/A/Doc/ -- another submodule
/home/bf/projects/A/Doc/technical -- a subdirectory of Doc submodule

and you are in /home/bf/projects/A/Doc/technical/ subdirectory, your
$git_dir variable (which is grossly misnamed, too; call it "top"
perhaps?) would be "/home/bf/projects/A/Doc" and then your
$parent_git variable (again, that is misnamed; call it
"parent_top"?) would be "/home/bf/projects/A".  The submodule path
to the submodule you are currently in is "Doc" (you learn it as the
difference between these two).

You can ask the top-level project the name of the submodule that is
currently at "Doc" with "submodule--helper name".  Unless the project
has moved it since it first added the submodule, the module name may
also be "Doc", but if it were moved, it may be different.

And that "module name" is a more useful thing than a hardcoded
string "sub:" to use in the prompt, I would think.

Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always

[Jeff King]

On Mon, Jan 30, 2017 at 01:58:10PM -0800, Junio C Hamano wrote:

> > When writing the test for git-tag, I realized that the option
> > --no-create-reflog to git-tag does not take precedence over
> > logAllRefUpdate=always. IOW the setting cannot be overridden on the 
> > command
> > line. Do you think this is a defect or would it not be desirable to 
> > have this
> > feature anyway?
> 
> "--no-create-reflog" should override the configuration set to "true"
> or "always".  Also "--create-reflog" should override the
> configuration set to "false".
> 
> If the problem was inherited from the original code before your
> change (e.g. you set logAllRefUpdates to true and then did
> "update-ref --no-create-reflog refs/heads/foo".  Does the code
> before your change ignore the command lne option and create a reflog
> for the branch?), then it would be ideal to fix the bug before this
> series as a preparatory fix.  If the problem was introduced by this
> patch set, then we would need a fix not to introduce it ;-)

I hadn't thought about that. I think "git branch --no-create-reflog" has
the same problem in the existing code.

I suspect nobody cares much in practice. Even if you say "don't create a
reflog now", if you have core.logAllRefUpdates turned on, then it's
likely that some _other_ operation will create the reflog later
accidentally (e.g., as soon as you "git checkout foo && git commit",
you'll get a reflog). I think you're fighting an uphill battle to turn
logAllRefUpdates on and then try to disable some reflogs selectively.

So I agree the current behavior is quietly broken, which is not good.
But I wonder if "--no-create-reflog" is really sane in the first place,
and whether we might be better off to simply disallow it.

-Peff

Re: difflame

[Jeff King]

On Mon, Jan 30, 2017 at 01:08:41PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Tue, Jan 17, 2017 at 11:24:02PM -0600, Edmundo Carmona Antoranz wrote:
> >
> >> For a very long time I had wanted to get the output of diff to include
> >> blame information as well (to see when something was added/removed).
> >
> > This is something I've wanted, too. The trickiest part, though, is
> > blaming deletions, because git-blame only tracks the origin of content,
> > not the origin of a change.
> 
> Hmph, this is a comment without looking at what difflame does
> internally, so you can ignore me if I am misunderstood what problem
> you are pointing out, but I am not sure how "tracks the origin of
> content" could be a problem.
> 
> If output from "git show" says this:
> 
>   --- a/file
>   +++ b/file
>   @@ -1,5 +1,6 @@
>a
>b
>   -c
>   +C
>   +D
>d
>e
> 
> in order to annotate lines 'a', 'b', 'd', and 'e' for their origin,
> you would run 'blame' on the commit the above output was taken from
> (or its parent---they are in the context so either would be OK).
> 
> You know where 'C' and 'D' came from already.  It's the commit you
> are feeding "git show".

I think the point (or at least as I understand it) is that the diff is
not "git show" for a particular commit. It could be part of a much
larger diff that covers many commits.

As a non-hypothetical instance, I have a fork of git.git that has
various enhancements. I want to feed those enhancements upstream. I need
to know which commits should be cherry-picked to get those various
enhancements.

Looking at "log origin..fork" tells me too many commits, because it
includes ones which aren't useful anymore. Either because they already
went upstream, or because they were cherry-picked to the fork and their
upstream counterparts merged (or even equivalent commits made upstream
that obsoleted the features).

Looking at "git diff origin fork" tells me what the actual differences
are, but it doesn't show me which commits are responsible for them.

I can "git blame" each individual line of the diff (starting with "fork"
as the tip), but that doesn't work for lines that no longer exist (i.e.,
when the interesting change is a deletion).

> In order to run blame to find where 'c' came from, you need to start
> at the _parent_ of the commit the above output came from, and the
> hunk header shows which line range to find the final 'c'.

So perhaps that explains my comment more. "blame" is not good for
finding which commit took away a line. I've tried using "blame
--reverse", but it shows you the parent of the commit you are looking
for, which is slightly annoying. :)

"git log -S" is probably a better tool for finding that.

-Peff

Re: [PATCH] merge-recursive: make "CONFLICT (rename/delete)" message show both paths

[Junio C Hamano]

Matt McCutchen  writes:

> The current message printed by "git merge-recursive" for a rename/delete
> conflict is like this:
>
> CONFLICT (rename/delete): new-path deleted in HEAD and renamed in
> other-branch. Version other-branch of new-path left in tree.
>
> To be more helpful, the message should show both paths of the rename and
> state that the deletion occurred at the old path, not the new path.  So
> change the message to the following format:
>
> CONFLICT (rename/delete): old-path deleted in HEAD and renamed to
> new-path in other-branch. Version other-branch of new-path left in tree.

Sounds like a sensible goal.

> Please check that my refactoring is indeed correct!  All the existing tests 
> pass
> for me, but the existing test coverage of these conflict messages looks poor.

This unfortunately is doing a bit too many things at once from that
point of view.  I need to reserve a solid quiet 20-minutes without
distraction to check it, which I am hoping to do tonight.

Thanks.

>
>  merge-recursive.c  | 117 
> ++---
>  t/t6045-merge-rename-delete.sh |  23 
>  2 files changed, 86 insertions(+), 54 deletions(-)
>  create mode 100755 t/t6045-merge-rename-delete.sh
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d327209..e8fce10 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1061,16 +1061,20 @@ static int merge_file_one(struct merge_options *o,
>  }
>  
>  static int handle_change_delete(struct merge_options *o,
> -  const char *path,
> +  const char *path, const char *old_path,
>const struct object_id *o_oid, int o_mode,
> -  const struct object_id *a_oid, int a_mode,
> -  const struct object_id *b_oid, int b_mode,
> +  const struct object_id *changed_oid,
> +  int changed_mode,
> +  const char *change_branch,
> +  const char *delete_branch,
>const char *change, const char *change_past)
>  {
> - char *renamed = NULL;
> + char *alt_path = NULL;
> + const char *update_path = path;
>   int ret = 0;
> +
>   if (dir_in_way(path, !o->call_depth, 0)) {
> - renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
> + update_path = alt_path = unique_path(o, path, change_branch);
>   }
>  
>   if (o->call_depth) {
> @@ -1081,43 +1085,43 @@ static int handle_change_delete(struct merge_options 
> *o,
>*/
>   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 tree."),
> -change, path, o->branch1, change_past,
> -o->branch2, o->branch2, path);
> - ret = update_file(o, 0, b_oid, b_mode, path);
> - } else {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> -"and %s in %s. Version %s of %s left in tree at 
> %s."),
> -change, path, o->branch1, change_past,
> -o->branch2, o->branch2, path, renamed);
> - ret = update_file(o, 0, b_oid, b_mode, renamed);
> - }
> + ret = update_file(o, 0, o_oid, o_mode, update_path);
>   } else {
> - if (!renamed) {
> - output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> -"and %s in %s. Version %s of %s left in tree."),
> -change, path, o->branch2, change_past,
> -o->branch1, o->branch1, path);
> + if (!alt_path) {
> + if (!old_path) {
> + output(o, 1, _("CONFLICT (%s/delete): %s 
> deleted in %s "
> +"and %s in %s. Version %s of %s left in 
> tree."),
> +change, path, delete_branch, change_past,
> +change_branch, change_branch, path);
> + } else {
> + output(o, 1, _("CONFLICT (%s/delete): %s 
> deleted in %s "
> +"and %s to %s in %s. Version %s of %s 
> left in tree."),
> +change, old_path, delete_branch, 
> change_past, path,
> +change_branch, change_branch, 

Re: [PATCH 0/5] introduce SWAP macro

[Junio C Hamano]

René Scharfe  writes:

> Exchanging the value of two variables requires declaring a temporary
> variable and repeating their names.  The swap macro in apply.c
> simplifies this for its callers without changing the compiled binary.
> Polish this macro and export it, then use it throughout the code to
> reduce repetition and hide the declaration of temporary variables.

All looked reasonable (modulo "swap tree2 and tree1???" ordering).
Also the object-id ones looked good.

Thanks.

Re: [PATCH] Completion: Add support for --submodule=diff

[Junio C Hamano]

Peter Law  writes:

>> Teach git-completion.bash about the 'diff' option to 'git diff
>> --submodule=', which was added in Git 2.11.
>
> I posted this patch back in December, but I've not heard anything. I'm
> sure as maintainers you're all quite busy, but I was wondering how
> long it usually takes to get a response to patches? (also whether I'd
> gotten some part of the submission process wrong?)

When there is clear "subsystem maintainer(s)" in the area, I try to
refrain from commenting until they speak up, and completion scripts
are one of these areas.  I usually ping them after a few days but in
December with holidays and things people are usually slow, and so
was I X-<.

Will pick it up.  Thanks for a reminder; you absolutely did the
right thing (i.e. sending it out with people who are likely to know
about the area CC'ed, waiting for a while and then sending a
reminder).

Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always

[Junio C Hamano]

Junio C Hamano  writes:

>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index bfe685c..81ea2ed 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct 
>> checkout_opts *opts,
>>  const char *old_desc, *reflog_msg;
>>  if (opts->new_branch) {
>>  if (opts->new_orphan_branch) {
>> -if (opts->new_branch_log && !log_all_ref_updates) {
>> +const char *refname = mkpathdup("refs/heads/%s", 
>> opts->new_orphan_branch);
>> ...
>>  if (ret) {
>>  fprintf(stderr, _("Can not do reflog 
>> for '%s': %s\n"),
>>  opts->new_orphan_branch, 
>> err.buf);
>
> Here you need to have another free(), as this block makes an early
> return and you end up leaking refname.

I am building with the attached patch squashed on top.  

The extra free(refname) is to plug the leak I pointed out, and the
type of refname is no longer const, because "const char *" cannot be
free()d without casting, and in this codepath I do not see a reason
to mark it as const.

When queued on top of 4e59582ff7 ("Seventh batch for 2.12",
2017-01-23), however, this fails t2017#9 (orphan with -l makes
reflog when core.logAllRefUpdates = false).

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 81ea2eda99..e1a60fd8ea 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -612,7 +612,9 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
const char *old_desc, *reflog_msg;
if (opts->new_branch) {
if (opts->new_orphan_branch) {
-   const char *refname = mkpathdup("refs/heads/%s", 
opts->new_orphan_branch);
+   char *refname;
+
+   refname = mkpathdup("refs/heads/%s", 
opts->new_orphan_branch);
if (opts->new_branch_log && 
should_autocreate_reflog(refname)) {
int ret;
struct strbuf err = STRBUF_INIT;
@@ -622,6 +624,7 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
fprintf(stderr, _("Can not do reflog 
for '%s': %s\n"),
opts->new_orphan_branch, 
err.buf);
strbuf_release();
+   free(refname);
return;
}
strbuf_release();

Re: [PATCH v4 0/5] urlmatch: allow wildcard-based matches

[Junio C Hamano]

Junio C Hamano  writes:

> Patrick Steinhardt  writes:
>
>>  - I realized that with my patches, "ranking" of URLs was broken.
>>Previously, we've always taken the longest matching URL. As
>>previously, only the user and path could actually differ, only
>>these two components were used for the comparison. I've
>>changed this now to also include the host part so that URLs
>>with a longer host will take precedence. This resulted in a
>>the patch 4.
>
> Good thinking.  I was wondering about this, too.
>
> Thanks.  Will read it through and replace.

Ugh.  When applied on top of 4e59582ff7 ("Seventh batch for 2.12",
2017-01-23), Git fails its self-test in t5551 #31 (I do not run with
EXPENSIVE so some tests liks #30 are skipped, if it matters).

Re: [PATCH] checkout: convert post_checkout_hook() to struct object_id

[brian m. carlson]

On Mon, Jan 30, 2017 at 02:01:12PM +0100, Johannes Schindelin wrote:
> Hi René,
> 
> On Sat, 28 Jan 2017, René Scharfe wrote:
> 
> > Signed-off-by: Rene Scharfe 
> 
> These three SHA-1 -> OID patches all appear correct to me.

I concur.  These look good.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 4/5] diff: use SWAP macro

[Junio C Hamano]

René Scharfe  writes:

> Use the macro SWAP to exchange the value of pairs of variables instead
> of swapping them manually with the help of a temporary variable.  The
> resulting code is shorter and easier to read.
>
> The two cases were not transformed by the semantic patch swap.cocci
> because it's extra careful and handles only cases where the types of all
> variables are the same

Ah, that explains my "huh?" in 3/5; thanks.

Re: [PATCH 3/5] use SWAP macro

[Junio C Hamano]

René Scharfe  writes:

>   if (tree2->flags & UNINTERESTING) {
> - struct object *tmp = tree2;
> - tree2 = tree1;
> - tree1 = tmp;
> + SWAP(tree2, tree1);

A human would have written this SWAP(tree1, tree2).

Not that I think such a manual fix-up should be made in _this_
patch, which may end up mixing mechanical conversion (which we may
want to keep reproducible) and hand tweaks.  But this swapped swap
reads somewhat silly.

> diff --git a/diff-no-index.c b/diff-no-index.c
> index f420786039..1ae09894d7 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -186,9 +186,8 @@ static int queue_diff(struct diff_options *o,
>  
>   if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
>   unsigned tmp;
> - const char *tmp_c;
>   tmp = mode1; mode1 = mode2; mode2 = tmp;
> - tmp_c = name1; name1 = name2; name2 = tmp_c;
> + SWAP(name1, name2);

Curious that mode swapping is left for a later iteration.

> diff --git a/diff.c b/diff.c
> index f08cd8e033..9de1ba264f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5118,13 +5118,11 @@ void diff_change(struct diff_options *options,
>  
>   if (DIFF_OPT_TST(options, REVERSE_DIFF)) {
>   unsigned tmp;
> - const unsigned char *tmp_c;
> - tmp = old_mode; old_mode = new_mode; new_mode = tmp;
> - tmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;
> + SWAP(old_mode, new_mode);
> + SWAP(old_sha1, new_sha1);
>   tmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;
>   new_sha1_valid = tmp;

So is this one.

> diff --git a/merge-recursive.c b/merge-recursive.c
> ...
> - tmp = ren2;
> - ren2 = ren1;
> - ren1 = tmp;
> + SWAP(ren2, ren1);

A human would have written this SWAP(ren1, ren2).

Re: [PATCH 1/5] add SWAP macro

[Brandon Williams]

On 01/30, René Scharfe wrote:
> Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:
> >It is curious, though, that an
> >expression like "sizeof(a++)" would not be rejected.
> 
> Clang normally warns about something like this ("warning: expression
> with side effects has no effect in an unevaluated context
> [-Wunevaluated-expression]"), but not if the code is part of a
> macro.  I don't know if that's intended, but it sure is helpful in
> the case of SWAP.
> 
> >Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?
> 
> That might be a valid expectation, but GCC says "error: lvalue
> required as unary '&' operand" and clang puts it "error: cannot take
> the address of an rvalue of type".
> 
> René

Perhaps we could disallow a side-effect operator in the macro.  By
disallow I mean place a comment at the definition to the macro and
hopefully catch something like that in code-review.  We have the same
issue with the `ALLOC_GROW()` macro.

-- 
Brandon Williams

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 30.01.2017 um 22:03 schrieb Johannes Schindelin:

It is curious, though, that an
expression like "sizeof(a++)" would not be rejected.


Clang normally warns about something like this ("warning: expression 
with side effects has no effect in an unevaluated context 
[-Wunevaluated-expression]"), but not if the code is part of a macro.  I 
don't know if that's intended, but it sure is helpful in the case of SWAP.



Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?


That might be a valid expectation, but GCC says "error: lvalue required 
as unary '&' operand" and clang puts it "error: cannot take the address 
of an rvalue of type".


René

Re: [PATCH v4 0/5] urlmatch: allow wildcard-based matches

[Junio C Hamano]

Patrick Steinhardt  writes:

>  - I realized that with my patches, "ranking" of URLs was broken.
>Previously, we've always taken the longest matching URL. As
>previously, only the user and path could actually differ, only
>these two components were used for the comparison. I've
>changed this now to also include the host part so that URLs
>with a longer host will take precedence. This resulted in a
>the patch 4.

Good thinking.  I was wondering about this, too.

Thanks.  Will read it through and replace.

Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always

[Junio C Hamano]

cornelius.w...@tngtech.com writes:

> Notes:
> Changes wrt v2:
> 
>  - change wording in commit message s/do not typically/are not meant to/;
>  - in update_refs_for_switch move refname to the enclosing block, so that
>should_autocreate_reflog has access. Thanks Junio for spotting this
>potential bug early :)
>  - add test that asserts reflogs are created for tags if
>logAllRefUpdates=always. The case with logAllRefUpdates=true is IMHO 
> already
>covered by the default case. To make that clearer, I explicitly added
>logAllRefUpdates=true.

These look all sensible.  Especially thanks for reordering the code
to feed the real refname for the new branch in the "checkout"
codepath.

> When writing the test for git-tag, I realized that the option
> --no-create-reflog to git-tag does not take precedence over
> logAllRefUpdate=always. IOW the setting cannot be overridden on the 
> command
> line. Do you think this is a defect or would it not be desirable to have 
> this
> feature anyway?

"--no-create-reflog" should override the configuration set to "true"
or "always".  Also "--create-reflog" should override the
configuration set to "false".

If the problem was inherited from the original code before your
change (e.g. you set logAllRefUpdates to true and then did
"update-ref --no-create-reflog refs/heads/foo".  Does the code
before your change ignore the command lne option and create a reflog
for the branch?), then it would be ideal to fix the bug before this
series as a preparatory fix.  If the problem was introduced by this
patch set, then we would need a fix not to introduce it ;-)

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfe685c..81ea2ed 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct 
> checkout_opts *opts,
>   const char *old_desc, *reflog_msg;
>   if (opts->new_branch) {
>   if (opts->new_orphan_branch) {
> - if (opts->new_branch_log && !log_all_ref_updates) {
> + const char *refname = mkpathdup("refs/heads/%s", 
> opts->new_orphan_branch);
> + if (opts->new_branch_log && 
> should_autocreate_reflog(refname)) {
>   int ret;
> - char *refname;
>   struct strbuf err = STRBUF_INIT;
>  
> - refname = mkpathdup("refs/heads/%s", 
> opts->new_orphan_branch);
>   ret = safe_create_reflog(refname, 1, );
> - free(refname);
>   if (ret) {
>   fprintf(stderr, _("Can not do reflog 
> for '%s': %s\n"),
>   opts->new_orphan_branch, 
> err.buf);

Here you need to have another free(), as this block makes an early
return and you end up leaking refname.

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 1cfa8a2..1bf622d 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -71,6 +71,7 @@ test_expect_success 'creating a tag for an unknown revision 
> should fail' '
>  
>  # commit used in the tests, test_tick is also called here to freeze the date:
>  test_expect_success 'creating a tag using default HEAD should succeed' '
> + test_config core.logAllRefUpdates true &&
>   test_tick &&
>   echo foo >foo &&
>   git add foo &&

This change is to make sure that 'true' does not affect tags (but
'always' does as seen in the later new test)?  I am just double
checking, not objecting.

Thanks.

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 30.01.2017 um 21:48 schrieb Johannes Schindelin:

So I tried to verify that Visual C optimizes this well, and oh my deity,
this was not easy. In Debug mode, it does not optimize, i.e. the memcpy()
will be called, even for simple 32-bit integers. In Release mode, Visual
Studio's defaults turn on "whole-program optimization" which means that
the only swapping that is going on in the end is that the meaning of two
registers is swapped, i.e. the SWAP() macro is expanded to... no assembler
code at all.

After trying this and that and something else, I finally ended up with the
file-scope optimized SWAP() resulting in this assembly code:

7FF791311000  mov eax,dword ptr [rcx]
7FF791311002  mov r8d,dword ptr [rdx]
7FF791311005  mov dword ptr [rcx],r8d
7FF791311008  mov dword ptr [rdx],eax


This looks good.


However, recent events (including some discussions on this mailing list
which had unfortunate consequences) made me trust my instincts more. And
my instincts tell me that it would be unwise to replace code that swaps
primitive types in the straight-forward way by code that relies on
advanced compiler optimization to generate efficient code, otherwise
producing very suboptimal code.


I don't know how difficult it was to arrive at the result above, but I 
wouldn't call inlining memcpy(3) an advanced optimization (anymore?), 
since the major compilers seem to be doing that.


The SWAP in prio-queue.c seems to be the one with the biggest 
performance impact.  Or perhaps it's the one in lookup_object()?  The 
former is easier to measure, though.


Here's what I get with CFLAGS="-builtin -O2" (best of five):

$ time ./t/helper/test-prio-queue $(seq 10 -1 1) dump >/dev/null

real0m0.142s
user0m0.120s
sys 0m0.020s

And this is with CFLAGS="-no-builtin -O2":

$ time ./t/helper/test-prio-queue $(seq 10 -1 1) dump >/dev/null

real0m0.170s
user0m0.156s
sys 0m0.012s

Hmm.  Not nice, but also not prohibitively slow.


The commit you quoted embarrasses me, and I have no excuse for it. I would
love to see that myswap() ugliness fixed by replacing it with a construct
that is simpler, and generates good code even without any smart compiler.


I don't see a way to do that without adding a type parameter.

René

Re: [PATCH] Completion: Add support for --submodule=diff

[Brandon Williams]

On 01/30, Peter Law wrote:
> Hi,
> 
> > Teach git-completion.bash about the 'diff' option to 'git diff
> > --submodule=', which was added in Git 2.11.
> 
> I posted this patch back in December, but I've not heard anything. I'm
> sure as maintainers you're all quite busy, but I was wondering how
> long it usually takes to get a response to patches? (also whether I'd
> gotten some part of the submission process wrong?)
> 
> Thanks,
> Peter

It looks like this must have just fallen through the cracks.  Your patch
looks good to me and works when I test it locally.

-- 
Brandon Williams

Re: [PATCH] Completion: Add support for --submodule=diff

[Peter Law]

Hi,

> Teach git-completion.bash about the 'diff' option to 'git diff
> --submodule=', which was added in Git 2.11.

I posted this patch back in December, but I've not heard anything. I'm
sure as maintainers you're all quite busy, but I was wondering how
long it usually takes to get a response to patches? (also whether I'd
gotten some part of the submission process wrong?)

Thanks,
Peter

Re: [PATCH v2 1/4] Documentation/stash: remove mention of git reset --hard

[Junio C Hamano]

Thomas Gummerer  writes:

> Don't mention git reset --hard in the documentation for git stash save.
> It's an implementation detail that doesn't matter to the end user and
> thus shouldn't be exposed to them.

Everybody understands what "reset --hard" does; it can be an
easier-to-read "short-hand" description to say "reset --hard"
instead of giving a lengthy description of what happens.

Because of that, I do not necessarily agree with the above
justification.  I'll read the remainder of the series before
commenting further on the above.

> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..0fc23c25ee 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -47,8 +47,9 @@ OPTIONS
>  
>  save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
> [-q|--quiet] []::
>  
> - Save your local modifications to a new 'stash', and run `git reset
> - --hard` to revert them.  The  part is optional and gives
> + Save your local modifications to a new 'stash' and roll them
> + back both in the working tree and in the index.

"... roll them back both ..." is unclear where they are rolled back
to.  

Perhaps "roll them back ... to HEAD" or something?

Re: [PATCH v2 2/4] stash: introduce push verb

[Junio C Hamano]

Thomas Gummerer  writes:

> Introduce a new git stash push verb in addition to git stash save.  The
> push verb is used to transition from the current command line arguments
> to a more conventional way, in which the message is specified after a -m
> parameter instead of being a positional argument.

I think the canonical way to express that is "... the message is
given as an argument to the -m option" (i.e. some options take an
argument, some others do not, and the "-m" takes one).

> This allows introducing a new filename argument to stash single files.

I do not want them to be "a filename argument", and I do not think
you meant them as such, either.  

This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say
which subset of paths to stash (and leave others behind).

> +save_stash () {
> + push_options=
> + while test $# != 0
> + do
> + case "$1" in
> + -k|--keep-index)
> +...
> + esac
> + shift
> + done

It is a bit unfortunate that we need to duplicate the above
case/esac here.  I do not know if doing it this way:

case "$1" in
--)
shift
break 
;;
--help)
show_help
;;
-*)
# pass all options through to push_stash
push_options="$push_options $1"
;;
*)
break
;;
esac

and letting push_stash complain for an unknown option is easier to
maintain.

You are reversing the order of the options in the loop.  Don't.

Re: [PATCH v2 4/4] stash: support filename argument

[Junio C Hamano]

Thomas Gummerer  writes:

> Add an optional filename argument to git stash push, which allows for
> stashing a single (or multiple) files.

You can give pathspec with one or more elements, so "an optional
argument" sounds too limiting.  

Allow 'git stash push' to take pathspec to specify which paths
to stash.

Also retitle

stash: teach 'push' (and 'create') to honor pathspec

or something.

> @@ -56,6 +61,10 @@ save [-p|--patch] [-k|--[no-]keep-index] 
> [-u|--include-untracked] [-a|--all] [-q
>   only  does not trigger this action to prevent a misspelled
>   subcommand from making an unwanted stash.
>  +
> +If the paths argument is given in 'git stash push', only these files
> +are put in the new 'stash'.  In addition only the indicated files are
> +changed in the working tree to match the index.

Actually the stash contains "all paths".  You could say that you are
placing _modifications_ to these paths in stash, even though that is
not how Git's world model works (i.e. everything is a snapshot, and
modifications are merely difference between two successive
snapshots).  A technically correct version may be:

When pathspec is given to 'git stash push', the new stash
records the modified states only for the files that match
the pathspec.  The index entries and working tree files are
then rolled back to the state in HEAD only for these files,
too, leaving files that do not match the pathspec intact.

> diff --git a/git-stash.sh b/git-stash.sh
> index 5f08b43967..0072a38b4c 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -41,7 +41,7 @@ no_changes () {
>  untracked_files () {
>   excl_opt=--exclude-standard
>   test "$untracked" = "all" && excl_opt=
> - git ls-files -o -z $excl_opt
> + git ls-files -o -z $excl_opt -- $1

Hmph, why "$1" is spelled without dq, implying that it is split at
$IFS boundary?  This line alone makes me suspect that this is not
prepared to deal correctly with $IFS.  Let's read on...

> @@ -59,6 +59,7 @@ create_stash () {
>   stash_msg=
>   untracked=
>   new_style=
> + files=
>   while test $# != 0
>   do
>   case "$1" in
> @@ -72,6 +73,12 @@ create_stash () {
>   untracked="$1"
>   new_style=t
>   ;;
> + --)
> + shift
> + files="$@"

Isn't this the same as writing files="$*", i.e. concatenate the
multiple arguments with the first whitespace in $IFS in between?

> @@ -134,7 +141,7 @@ create_stash () {
>   # Untracked files are stored by themselves in a parentless 
> commit, for
>   # ease of unpacking later.
>   u_commit=$(
> - untracked_files | (
> + untracked_files $files | (

... and this lets it split at $IFS again when passing it down to the
helper.  But the helper looks only at $1 so the second and subsequent
ones will be ignored altogether.

This cannot be correct, and any hunk in the remainder of the patch
that mentions $files will be incorrect for the same reason.

Is it possible to carry what the caller (and the end user) gave you
in "$@" without molesting it at all?  That would mean you do not
need to introduce $files variable at all, and then the places that
do things like this:

> - create_stash -m "$stash_msg" -u "$untracked"
> + create_stash -m "$stash_msg" -u "$untracked" -- $files

can instead do

create_stash -m "$stash_msg" -u "$untracked" -- "$@"

That would allow you to work correctly with pathspec with $IFS
whitespaces in them.

Re: [PATCH v2 3/4] introduce new format for git stash create

[Junio C Hamano]

Thomas Gummerer  writes:

>  create_stash () {
> - stash_msg="$1"
> - untracked="$2"
> + stash_msg=
> + untracked=
> + new_style=
> ...
> + while test $# != 0
> + do
> + case "$1" in
> + -m|--message)
> + shift
> + stash_msg="$1"

${1?"-m needs an argument"}

to error check "git stash create -m"?

> + if test -z "$new_style"
> + then
> + stash_msg="$*"
> + fi

This breaks external users who do "git stash create" in the old
fashioned way, I think, but can be easily fixed with something like:

stash_msg=$1 untracked=$2

If the existing tests did not catch this, I guess there is a
coverage gap we may want to fill.  Perhaps add a new test to 3903
that runs "git stash create message untracked" and makes sure it
still works?

Re: [PATCH 1/5] add SWAP macro

[Johannes Schindelin]

Hi Hannes,

On Mon, 30 Jan 2017, Johannes Sixt wrote:

> Am 30.01.2017 um 17:01 schrieb Johannes
> Schindelin:
> > On Sat, 28 Jan 2017, René Scharfe wrote:
> > > diff --git a/git-compat-util.h
> > > b/git-compat-util.h
> > > index 87237b092b..66cd466eea 100644
> > > --- a/git-compat-util.h
> > > +++ b/git-compat-util.h
> > > @@ -527,6 +527,16 @@ static inline int
> > > @@ ends_with(const char *str, const char
> > > @@ *suffix)
> > >   return strip_suffix(str, suffix, );
> > >  }
> > >
> > > +#define SWAP(a, b) do {
> > > \
> > > + void *_swap_a_ptr = &(a);
> > > \
> > > + void *_swap_b_ptr = &(b);
> > > \
> > > + unsigned char _swap_buffer[sizeof(a)];
> > > \
> > > + memcpy(_swap_buffer, _swap_a_ptr,
> > > sizeof(a));   \
> > > + memcpy(_swap_a_ptr, _swap_b_ptr,
> > > sizeof(a) +   \
> > > +BUILD_ASSERT_OR_ZERO(sizeof(a)
> > > == sizeof(b)));   \
> > > + memcpy(_swap_b_ptr, _swap_buffer,
> > > sizeof(a));   \
> > > +} while (0)
> > > +
> > >  #if defined(NO_MMAP) ||
> > >  defined(USE_WIN32_MMAP)
> >
> > It may seem as a matter of taste, or maybe
> > not: I prefer this without the
> > _swap_a_ptr
> 
> The purpose of these pointers is certainly to
> avoid that the macro arguments are evaluated
> more than once.

I mistook "a" being used in sizeof(a) for breaking that assumption, but of
course a is *not* evaluated in that case. It is curious, though, that an
expression like "sizeof(a++)" would not be rejected.

Further, what would SWAP(a++, b) do? Swap a and b, and *then* increment a?

Ciao,
Johannes

Re: difflame

[Junio C Hamano]

Jeff King  writes:

> On Tue, Jan 17, 2017 at 11:24:02PM -0600, Edmundo Carmona Antoranz wrote:
>
>> For a very long time I had wanted to get the output of diff to include
>> blame information as well (to see when something was added/removed).
>
> This is something I've wanted, too. The trickiest part, though, is
> blaming deletions, because git-blame only tracks the origin of content,
> not the origin of a change.

Hmph, this is a comment without looking at what difflame does
internally, so you can ignore me if I am misunderstood what problem
you are pointing out, but I am not sure how "tracks the origin of
content" could be a problem.

If output from "git show" says this:

--- a/file
+++ b/file
@@ -1,5 +1,6 @@
 a
 b
-c
+C
+D
 d
 e

in order to annotate lines 'a', 'b', 'd', and 'e' for their origin,
you would run 'blame' on the commit the above output was taken from
(or its parent---they are in the context so either would be OK).

You know where 'C' and 'D' came from already.  It's the commit you
are feeding "git show".

In order to run blame to find where 'c' came from, you need to start
at the _parent_ of the commit the above output came from, and the
hunk header shows which line range to find the final 'c'.

So...

Re: [RFC] Proof of concept: Support multiple authors

[Junio C Hamano]

Christian Couder  writes:

> I am just wondering if you have read and taken into account the
> previous threads on this mailing list about the same subject, like for
> example this one:
>
> https://public-inbox.org/git/caovwq4i_hl7xgnxzrvu3osnsbntyxbg8vh6vzi4c1issrre...@mail.gmail.com/

Thanks for a starting-point link.

In that discussion, another discussion in the debian BTS is
mentioned,

  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880

which in turn has links to other, even earlier, discussions, but
they are all gmane links so it would be harder for those who do not
use its NNTP interface (which still works).  Here are their modern
counterparts ;-)

  https://public-inbox.org/git/?q=gmane:83880
  https://public-inbox.org/git/?q=gmane:146223
  https://public-inbox.org/git/?q=gmane:146886

The older discussions already cited the cost to update both in-tree
and out-of-tree tools not to barf when they see such a commit object
and one of the reason why we would not want to do this, and Ted Ts'o
gave us another excellent reason why not to do multiple author
header lines in a commit object header, i.e. "How often is that all
of the authors are completely equal?"

Another thing that I didn't see brought up was that it is not enough
to ensure all existing tools are updated not to barf on a commit
with multiple "author" line.  You need to define what it means to
have multiple authors and how they are treated by tools in a
consistent way.  Think "shortlog", for example.  The tool may be
able to be tweaked not to barf, and you may be able to add a random
definition of which of the multiple authors to group a single commit
under (the "random definition" could be "show that same commit
multiple times, once for each author" or it could be "concatenate
the authors to create a single header to list co-authored commits
under, as if they were a single person", or it could be something
equally bogus), but I do not think any single solution satisfies
people's needs, and my gut feeling is that it is not worth to add
tons of options and explain them to the end-users to support these
random things that happen when there are multiple authors.

Incidentally, there recently was a discussion on extending
request-pull by adding a summary of commits by reviewers and
testers.

https://public-inbox.org/git/20170115183051.3565-1-...@the-dreams.de/

I would imagine, if it is to be implemented, it would boil down to
teaching shortlog that the "author" line in the commit object header
is not the only thing that matters, and that it should optionally
take notice of lines in the trailer block of the log message (e.g.
perhaps "Co-Authored-by: " trailer suggested by $gmane/146223 above
would help there).

My advice to those who want to record credit to multiple authors is
to treat the commit author line as recording the primary contact
when there is a question on the commit and nothing else.  Anything
with richer semantics is better done in the trailer by enriching the
support of trailer lines with interpret-trailers framework.

Re: [PATCH 1/5] add SWAP macro

[Johannes Schindelin]

Hi René,

On Mon, 30 Jan 2017, René Scharfe wrote:

> Am 30.01.2017 um 16:39 schrieb Johannes Schindelin:
>
> > On Sat, 28 Jan 2017, René Scharfe wrote:
> >
> > > Add a macro for exchanging the values of variables.  It allows users
> > > to avoid repetition and takes care of the temporary variable for
> > > them.  It also makes sure that the storage sizes of its two
> > > parameters are the same.  Its memcpy(1) calls are optimized away by
> > > current compilers.
> >
> > How certain are we that "current compilers" optimize that away? And
> > about which "current compilers" are we talking? GCC? GCC 6? Clang?
> > Clang 3?  Clang 3.8.*? Visual C 2005+?
> 
> GCC 4.4.7 and clang 3.2 on x86-64 compile the macro to the same object
> code as a manual swap ; see https://godbolt.org/g/F4b9M9.  Both were
> released 2012.

Good. Thank you.

> That website doesn't offer Visual C(++), but since you added the
> original macro in e5a94313c0 ("Teach git-apply about '-R'", 2006) I
> guess we have that part covered. ;)

Back then, I was not able to build Git using Visual C *at all*. It
required a Herculean effort to make that happen, and it did not happen
before the Git for Windows project was started in 2007.

So I tried to verify that Visual C optimizes this well, and oh my deity,
this was not easy. In Debug mode, it does not optimize, i.e. the memcpy()
will be called, even for simple 32-bit integers. In Release mode, Visual
Studio's defaults turn on "whole-program optimization" which means that
the only swapping that is going on in the end is that the meaning of two
registers is swapped, i.e. the SWAP() macro is expanded to... no assembler
code at all.

After trying this and that and something else, I finally ended up with the
file-scope optimized SWAP() resulting in this assembly code:

7FF791311000  mov eax,dword ptr [rcx]  
7FF791311002  mov r8d,dword ptr [rdx]  
7FF791311005  mov dword ptr [rcx],r8d  
7FF791311008  mov dword ptr [rdx],eax  

However, recent events (including some discussions on this mailing list
which had unfortunate consequences) made me trust my instincts more. And
my instincts tell me that it would be unwise to replace code that swaps
primitive types in the straight-forward way by code that relies on
advanced compiler optimization to generate efficient code, otherwise
producing very suboptimal code.

The commit you quoted embarrasses me, and I have no excuse for it. I would
love to see that myswap() ugliness fixed by replacing it with a construct
that is simpler, and generates good code even without any smart compiler.

Ciao,
Dscho

[PATCH 0/4] git-prompt.sh: Full patch for submodule indicator

[Benjamin Fuchs]

Hi everyone,
since I didn't get a response I decided to sent my patch again. Maybe it was 
because
I to sent my consecutive commits the wrong way, so a new try.
First thanks again Steffen and Gábor for your feedback.
Based on the first feedback I rework the indicator and it is now way cheaper 
then the
first version and has a 'dirty' indicator now.
Tests exist also now.
Looking forward to more feedback!
Greetings,
Benjamin

Benjamin Fuchs (4):
  git-prompt.sh: add submodule indicator
  git-prompt.sh: rework of submodule indicator
  git-prompt.sh: fix for submodule 'dirty' indicator
  git-prompt.sh: add tests for submodule indicator

 contrib/completion/git-prompt.sh | 36 -
 t/t9903-bash-prompt.sh   | 43 
 2 files changed, 78 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH 4/4] git-prompt.sh: add tests for submodule indicator

[Benjamin Fuchs]

Signed-off-by: Benjamin Fuchs 
---
 t/t9903-bash-prompt.sh | 43 +++
 1 file changed, 43 insertions(+)

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 97c9b32..4dce366 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -37,6 +37,11 @@ test_expect_success 'setup for prompt tests' '
git commit -m "yet another b2" file &&
mkdir ignored_dir &&
echo "ignored_dir/" >>.gitignore &&
+   git checkout -b submodule &&
+   git submodule add ./. sub &&
+   git -C sub checkout master &&
+   git add sub &&
+   git commit -m submodule &&
git checkout master
 '
 
@@ -755,4 +760,42 @@ test_expect_success 'prompt - hide if pwd ignored - inside 
gitdir (stderr)' '
test_cmp expected "$actual"
 '
 
+test_expect_success 'prompt - submodule indicator' '
+   printf " (sub:master)" >expected &&
+   git checkout submodule &&
+   test_when_finished "git checkout master" &&
+   (
+   cd sub &&
+   GIT_PS1_SHOWSUBMODULE=1 &&
+   __git_ps1 >"$actual"
+   ) &&
+   test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - submodule indicator - verify false' '
+   printf " (master)" >expected &&
+   git checkout submodule &&
+   test_when_finished "git checkout master" &&
+   (
+   cd sub &&
+   GIT_PS1_SHOWSUBMODULE= &&
+   __git_ps1 >"$actual"
+   ) &&
+   test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - submodule indicator - dirty status indicator' '
+   printf " (+sub:b1)" >expected &&
+   git checkout submodule &&
+   git -C sub checkout b1 &&
+   test_when_finished "git checkout master" &&
+   (
+   cd sub &&
+   GIT_PS1_SHOWSUBMODULE=1 &&
+   __git_ps1 >"$actual"
+   ) &&
+   test_cmp expected "$actual"
+'
+
+
 test_done
-- 
2.7.4

[PATCH 3/4] git-prompt.sh: fix for submodule 'dirty' indicator

[Benjamin Fuchs]

Fixing wrong git diff line.

Signed-off-by: Benjamin Fuchs 
---
 contrib/completion/git-prompt.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c44b9a2..43b28e9 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -306,9 +306,9 @@ __git_ps1_submodule ()
local submodule_name="$(basename "$git_dir")"
if [ "$submodule_name" != ".git" ] && [ "$submodule_name" != "." ]; then
local parent_top="${git_dir%.git*}"
-   local submodule_top="${git_dir#*modules}"
+   local submodule_top="${git_dir#*modules/}"
local status=""
-   git diff -C "$parent_top" --no-ext-diff 
--ignore-submodules=dirty --quiet -- "$submodule_top" 2>/dev/null || status="+"
+   git -C "$parent_top" diff --no-ext-diff 
--ignore-submodules=dirty --quiet -- "$submodule_top" 2>/dev/null || status="+"
printf "$status$submodule_name:"
fi
 }
@@ -544,7 +544,7 @@ __git_ps1 ()
 
local sub=""
if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
-   sub="$(__git_ps1_submodule $g)"
+   sub="$(__git_ps1_submodule "$g")"
fi
 
local f="$w$i$s$u"
-- 
2.7.4

[PATCH 2/4] git-prompt.sh: rework of submodule indicator

[Benjamin Fuchs]

Rework of the first patch. The prompt now will look like this:
(+name:master). I tried to considere all suggestions.
Tests still missing.

Signed-off-by: Benjamin Fuchs 
---
 contrib/completion/git-prompt.sh | 49 
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 4c82e7f..c44b9a2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -95,8 +95,10 @@
 # repository level by setting bash.hideIfPwdIgnored to "false".
 #
 # If you would like __git_ps1 to indicate that you are in a submodule,
-# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
-# the branch name.
+# set GIT_PS1_SHOWSUBMODULE to a nonempty value. In this case the name
+# of the submodule will be prepended to the branch name (e.g. module:master).
+# The name will be prepended by "+" if the currently checked out submodule
+# commit does not match the SHA-1 found in the index of the containing 
repository.
 
 # check whether printf supports -v
 __git_printf_supports_v=
@@ -288,30 +290,27 @@ __git_eread ()
test -r "$f" && read "$@" <"$f"
 }
 
-# __git_is_submodule
-# Based on:
-# 
http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
-__git_is_submodule ()
-{
-   local git_dir parent_git module_name path
-   # Find the root of this git repo, then check if its parent dir is also 
a repo
-   git_dir="$(git rev-parse --show-toplevel)"
-   module_name="$(basename "$git_dir")"
-   parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> 
/dev/null)"
-   if [[ -n $parent_git ]]; then
-   # List all the submodule paths for the parent repo
-   while read path
-   do
-   if [[ "$path" != "$module_name" ]]; then continue; fi
-   if [[ -d "$git_dir/../$path" ]];then return 0; fi
-   done < <(cd $parent_git && git submodule --quiet foreach 'echo 
$path' 2> /dev/null)
-fi
-return 1
-}
-
+# __git_ps1_submodule
+#
+# $1 - git_dir path
+#
+# Returns the name of the submodule if we are currently inside one. The name
+# will be prepended by "+" if the currently checked out submodule commit does
+# not match the SHA-1 found in the index of the containing repository.
+# NOTE: git_dir looks like in a ...
+# - submodule: "GIT_PARENT/.git/modules/PATH_TO_SUBMODULE"
+# - parent: "GIT_PARENT/.git" (exception "." in .git)
 __git_ps1_submodule ()
 {
-   __git_is_submodule && printf "sub:"
+   local git_dir="$1"
+   local submodule_name="$(basename "$git_dir")"
+   if [ "$submodule_name" != ".git" ] && [ "$submodule_name" != "." ]; then
+   local parent_top="${git_dir%.git*}"
+   local submodule_top="${git_dir#*modules}"
+   local status=""
+   git diff -C "$parent_top" --no-ext-diff 
--ignore-submodules=dirty --quiet -- "$submodule_top" 2>/dev/null || status="+"
+   printf "$status$submodule_name:"
+   fi
 }
 
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
@@ -545,7 +544,7 @@ __git_ps1 ()
 
local sub=""
if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
-   sub="$(__git_ps1_submodule)"
+   sub="$(__git_ps1_submodule $g)"
fi
 
local f="$w$i$s$u"
-- 
2.7.4

[PATCH 1/4] git-prompt.sh: add submodule indicator

[Benjamin Fuchs]

I expirienced that working with submodules can be confusing. This indicator
will make you notice very easy when you switch into a submodule.
The new prompt will look like this: (sub:master)
Adding a new optional env variable for the new feature.

Signed-off-by: Benjamin Fuchs 
---
 contrib/completion/git-prompt.sh | 37 -
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 97eacd7..4c82e7f 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -93,6 +93,10 @@
 # directory is set up to be ignored by git, then set
 # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
 # repository level by setting bash.hideIfPwdIgnored to "false".
+#
+# If you would like __git_ps1 to indicate that you are in a submodule,
+# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
+# the branch name.
 
 # check whether printf supports -v
 __git_printf_supports_v=
@@ -284,6 +288,32 @@ __git_eread ()
test -r "$f" && read "$@" <"$f"
 }
 
+# __git_is_submodule
+# Based on:
+# 
http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
+__git_is_submodule ()
+{
+   local git_dir parent_git module_name path
+   # Find the root of this git repo, then check if its parent dir is also 
a repo
+   git_dir="$(git rev-parse --show-toplevel)"
+   module_name="$(basename "$git_dir")"
+   parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> 
/dev/null)"
+   if [[ -n $parent_git ]]; then
+   # List all the submodule paths for the parent repo
+   while read path
+   do
+   if [[ "$path" != "$module_name" ]]; then continue; fi
+   if [[ -d "$git_dir/../$path" ]];then return 0; fi
+   done < <(cd $parent_git && git submodule --quiet foreach 'echo 
$path' 2> /dev/null)
+fi
+return 1
+}
+
+__git_ps1_submodule ()
+{
+   __git_is_submodule && printf "sub:"
+}
+
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
 # when called from PS1 using command substitution
 # in this mode it prints text to add to bash PS1 prompt (includes branch name)
@@ -513,8 +543,13 @@ __git_ps1 ()
b="\${__git_ps1_branch_name}"
fi
 
+   local sub=""
+   if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
+   sub="$(__git_ps1_submodule)"
+   fi
+
local f="$w$i$s$u"
-   local gitstring="$c$b${f:+$z$f}$r$p"
+   local gitstring="$c$sub$b${f:+$z$f}$r$p"
 
if [ $pcmode = yes ]; then
if [ "${__git_printf_supports_v-}" != yes ]; then
-- 
2.7.4

Re: [RFC] Proof of concept: Support multiple authors

[Cornelius Schumacher]

On Monday 30 January 2017 18:56:42 Christian Couder wrote:
> On Sun, Jan 29, 2017 at 7:06 PM, Cornelius Schumacher
>  wrote:
> > This patch is a proof of concept implementation of support for
> > multiple authors. It adds an optional `authors` header to commits
> > which is set when there are authors configured in the git config.
> 
> I am just wondering if you have read and taken into account the
> previous threads on this mailing list about the same subject, like for
> example this one:
> 
> https://public-inbox.org/git/CAOvwQ4i_HL7XGnxZrVu3oSnsbnTyxbg8Vh6vzi4c1isSrr
> e...@mail.gmail.com/

Thanks for the pointer. I have read what I could find about the topic and 
tried to take it into account. Conceptually I wouldn't want to alter the 
semantics of the existing author field, but add optional information to 
capture the nature of commits done by multiple people collaboratively, where 
attribution to a single author is not an adequate representation of how the 
commit was done.

Maybe it still would be too intrusive to add an additional header, and there 
would be more elegant solutions to this problem. I would be very much 
interested to hear about better ideas how to handle this. On the other hand it 
seems to be the most straight-forward solution to handle this on the same 
level as single author information. But maybe this is due to my still limited 
familiarity to the internals of git ;-)

What I know from the experience of pair programming is that it is an actual 
problem to not be able to represent this information in a native way. It would 
benefit quite a number of programmers to improve that. I'm trying to find a 
solution which does that and still is compatible with the design of git. Any 
comments leading to an acceptable solution I highly appreciate.

> > Adding support for multiple authors would make the life of developers
> > doing
> > pair programming easier. It would be useful in itself, but it would also
> > need support by other tools around git to use its full potential.
> 
> From what I recall from previous discussions, the most important
> question is: are you sure that it doesn't break any other tool?

I have tried with a few tools and didn't find breakage other than that the 
additional information would not be taken into account. That of course doesn't 
mean that we could be sure that there are no tools which would break. Does 
anybody have hints on what tools would be most sensitive to such a change?

I realize that it does take effort and time to implement such a feature in a 
way which doesn't create breakage. But I still would like to try how far we 
could come with that., because maybe it actually can be done.

-- 
Cornelius Schumacher 

Re: [PATCH 1/5] add SWAP macro

[Johannes Sixt]

Am 30.01.2017 um 17:01 schrieb Johannes Schindelin:

On Sat, 28 Jan 2017, René Scharfe wrote:

diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092b..66cd466eea 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char 
*suffix)
return strip_suffix(str, suffix, );
 }

+#define SWAP(a, b) do {\
+   void *_swap_a_ptr = &(a);   \
+   void *_swap_b_ptr = &(b);   \
+   unsigned char _swap_buffer[sizeof(a)];  \
+   memcpy(_swap_buffer, _swap_a_ptr, sizeof(a));   \
+   memcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\
+  BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));   \
+   memcpy(_swap_b_ptr, _swap_buffer, sizeof(a));   \
+} while (0)
+
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)


It may seem as a matter of taste, or maybe not: I prefer this without the
_swap_a_ptr


The purpose of these pointers is certainly to avoid that the macro 
arguments are evaluated more than once.


-- Hannes

Re: [RFC] Proof of concept: Support multiple authors

[Christian Couder]

Hi,

On Sun, Jan 29, 2017 at 7:06 PM, Cornelius Schumacher
 wrote:
> This patch is a proof of concept implementation of support for
> multiple authors. It adds an optional `authors` header to commits
> which is set when there are authors configured in the git config.

I am just wondering if you have read and taken into account the
previous threads on this mailing list about the same subject, like for
example this one:

https://public-inbox.org/git/caovwq4i_hl7xgnxzrvu3osnsbntyxbg8vh6vzi4c1issrre...@mail.gmail.com/

> A new command `git-authors` is used to manage the authors settings.
> Authors are identified by initials and their names and emails are
> set in a `.git_authors_map` file.
>
> Signed-off-by: Cornelius Schumacher 
> ---
>
> When doing pair programming we have to work around the limitation that
> git can only have a single author in each commit. There are some tools
> which help with that such as [git-duet] [1], but there are still some
> limits, because the information about multiple authors is not reflected
> in the native git data model.
>
> Here is a proposal how to change that and implement native support for
> multiple authors in git. It comes with a patch as a proof of concept.
> The patch by no means is finished, it doesn't cover all cases and needs
> more tests and error handling. It's meant as an illustration of the
> concept.
>
> The basic idea is to introduce a new optional `authors` header in
> commits which contains a list of authors. The header is set in each new
> commit when there is an entry `authors.current` in the git config listing
> the current authors. When this config is not there the behavior falls
> back to the current standard behavior.
>
> When the header is there it is treated in the same way as the author
> header. It's preserved on merges and similar operations, is displayed in
> git show, and used to create a list of `From` addresses in `format-patch`.
> Email supports multiple `From` addresses as specified in section 3.6.2 of
> RFC 5322.
>
> When multiple authors are configured, they still write the standard author
> header to keep backwards compatibility. The first author is used as author
> and committer. In the future it might be good to implement something like
> automatic rotation of the order of authors to give credit in a fair way.
>
> To make it easier to work with the authors there is a new command
> `git-authors`. It sets the list of authors using initials as shortcut for
> the full configuration with name and email. The mapping of initials to
> names and email addresses is taken from a file `.git_authors_map` in the
> home directory of the users. This way it's possible to quickly set a list
> of authors by running a command such as `git authors ab cd`. This is
> useful when doing pair programming because the people working together
> usually switch quite frequently and using the command with the intials is
> quicker and less error-prone than editing the configuration with full
> names and emails.
>
> The command also supports setting a single author, setting more than two
> authors or clearing the configuration for multiple authors to go back to
> the standard behavior without the new authors header.
>
> The concept of the command and the mappings file is similar to what
> git-duet does, so that it should be familiar to many people doing pair
> programming. The behavior of git doesn't change when the new feature is
> not used and when it's used it should be backwards compatible so that it
> doesn't break existing functionality. This should make a smooth transition
> for users who choose to make use of it.
>
> Adding support for multiple authors would make the life of developers doing
> pair programming easier. It would be useful in itself, but it would also
> need support by other tools around git to use its full potential.

>From what I recall from previous discussions, the most important
question is: are you sure that it doesn't break any other tool?

> This
> might take a while, but I think it's worth the effort.
>
> I'm willing to continue to work on this and create a patch which is suitable
> for inclusion in git.

[PATCH v3 16/27] attr: convert git_all_attrs() to use "struct attr_check"

[Brandon Williams]

From: Junio C Hamano 

This updates the other two ways the attribute check is done via an
array of "struct attr_check_item" elements.  These two niches
appear only in "git check-attr".

 * The caller does not know offhand what attributes it wants to ask
   about and cannot use attr_check_initl() to prepare the
   attr_check structure.

 * The caller may not know what attributes it wants to ask at all,
   and instead wants to learn everything that the given path has.

Such a caller can call attr_check_alloc() to allocate an empty
attr_check, and then call attr_check_append() to add attribute names
one by one.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Brandon Williams 
---

This is the correct 16/27 patch that doesn't have the rebase mistake discoverd
by Stefan.

 attr.c   | 30 +-
 attr.h   |  9 +++-
 builtin/check-attr.c | 60 ++--
 3 files changed, 43 insertions(+), 56 deletions(-)

diff --git a/attr.c b/attr.c
index e3298516a..40818246f 100644
--- a/attr.c
+++ b/attr.c
@@ -906,32 +906,22 @@ int git_check_attrs(const char *path, int num, struct 
attr_check_item *check)
return 0;
 }
 
-int git_all_attrs(const char *path, int *num, struct attr_check_item **check)
+void git_all_attrs(const char *path, struct attr_check *check)
 {
-   int i, count, j;
+   int i;
 
-   collect_some_attrs(path, 0, NULL);
+   attr_check_reset(check);
+   collect_some_attrs(path, check->nr, check->items);
 
-   /* Count the number of attributes that are set. */
-   count = 0;
-   for (i = 0; i < attr_nr; i++) {
-   const char *value = check_all_attr[i].value;
-   if (value != ATTR__UNSET && value != ATTR__UNKNOWN)
-   ++count;
-   }
-   *num = count;
-   ALLOC_ARRAY(*check, count);
-   j = 0;
for (i = 0; i < attr_nr; i++) {
+   const char *name = check_all_attr[i].attr->name;
const char *value = check_all_attr[i].value;
-   if (value != ATTR__UNSET && value != ATTR__UNKNOWN) {
-   (*check)[j].attr = check_all_attr[i].attr;
-   (*check)[j].value = value;
-   ++j;
-   }
+   struct attr_check_item *item;
+   if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+   continue;
+   item = attr_check_append(check, git_attr(name));
+   item->value = value;
}
-
-   return 0;
 }
 
 int git_check_attr(const char *path, struct attr_check *check)
diff --git a/attr.h b/attr.h
index e611b139a..9f2729842 100644
--- a/attr.h
+++ b/attr.h
@@ -56,13 +56,10 @@ int git_check_attrs(const char *path, int, struct 
attr_check_item *);
 extern int git_check_attr(const char *path, struct attr_check *check);
 
 /*
- * Retrieve all attributes that apply to the specified path.  *num
- * will be set to the number of attributes on the path; **check will
- * be set to point at a newly-allocated array of git_attr_check
- * objects describing the attributes and their values.  *check must be
- * free()ed by the caller.
+ * Retrieve all attributes that apply to the specified path.
+ * check holds the attributes and their values.
  */
-int git_all_attrs(const char *path, int *num, struct attr_check_item **check);
+extern void git_all_attrs(const char *path, struct attr_check *check);
 
 enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 889264a5b..40cdff13e 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,12 +24,13 @@ static const struct option check_attr_options[] = {
OPT_END()
 };
 
-static void output_attr(int cnt, struct attr_check_item *check,
-   const char *file)
+static void output_attr(struct attr_check *check, const char *file)
 {
int j;
+   int cnt = check->nr;
+
for (j = 0; j < cnt; j++) {
-   const char *value = check[j].value;
+   const char *value = check->items[j].value;
 
if (ATTR_TRUE(value))
value = "set";
@@ -42,36 +43,38 @@ static void output_attr(int cnt, struct attr_check_item 
*check,
printf("%s%c" /* path */
   "%s%c" /* attrname */
   "%s%c" /* attrvalue */,
-  file, 0, git_attr_name(check[j].attr), 0, value, 
0);
+  file, 0,
+  git_attr_name(check->items[j].attr), 0, value, 
0);
} else {
quote_c_style(file, NULL, stdout, 0);
-   printf(": %s: %s\n", git_attr_name(check[j].attr), 
value);
+   printf(": %s: 

[PATCH v3 15/27] attr: (re)introduce git_check_attr() and struct attr_check

[Brandon Williams]

From: Junio C Hamano 

A common pattern to check N attributes for many paths is to

 (1) prepare an array A of N attr_check_item items;
 (2) call git_attr() to intern the N attribute names and fill A;
 (3) repeatedly call git_check_attrs() for path with N and A;

A look-up for these N attributes for a single path P scans the
entire attr_stack, starting from the .git/info/attributes file and
then .gitattributes file in the directory the path P is in, going
upwards to find .gitattributes file found in parent directories.

An earlier commit 06a604e6 (attr: avoid heavy work when we know the
specified attr is not defined, 2014-12-28) tried to optimize out
this scanning for one trivial special case: when the attribute being
sought is known not to exist, we do not have to scan for it.  While
this may be a cheap and effective heuristic, it would not work well
when N is (much) more than 1.

What we would want is a more customized way to skip irrelevant
entries in the attribute stack, and the definition of irrelevance
is tied to the set of attributes passed to git_check_attrs() call,
i.e. the set of attributes being sought.  The data necessary for
this optimization needs to live alongside the set of attributes, but
a simple array of git_attr_check_elem simply does not have any place
for that.

Introduce "struct attr_check" that contains N, the number of
attributes being sought, and A, the array that holds N
attr_check_item items, and a function git_check_attr() that
takes a path P and this structure as its parameters.  This structure
can later be extended to hold extra data necessary for optimization.

Also, to make it easier to write the first two steps in common
cases, introduce git_attr_check_initl() helper function, which takes
a NULL-terminated list of attribute names and initialize this
structure.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Brandon Williams 
---

This is the correct 15/27 patch that doesn't have the rebase mistake discovered
by Stefan.

 attr.c | 74 ++
 attr.h | 17 +++
 2 files changed, 91 insertions(+)

diff --git a/attr.c b/attr.c
index 2f180d609..e3298516a 100644
--- a/attr.c
+++ b/attr.c
@@ -370,6 +370,75 @@ static void free_attr_elem(struct attr_stack *e)
free(e);
 }
 
+struct attr_check *attr_check_alloc(void)
+{
+   return xcalloc(1, sizeof(struct attr_check));
+}
+
+struct attr_check *attr_check_initl(const char *one, ...)
+{
+   struct attr_check *check;
+   int cnt;
+   va_list params;
+   const char *param;
+
+   va_start(params, one);
+   for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
+   ;
+   va_end(params);
+
+   check = attr_check_alloc();
+   check->nr = cnt;
+   check->alloc = cnt;
+   check->items = xcalloc(cnt, sizeof(struct attr_check_item));
+
+   check->items[0].attr = git_attr(one);
+   va_start(params, one);
+   for (cnt = 1; cnt < check->nr; cnt++) {
+   const struct git_attr *attr;
+   param = va_arg(params, const char *);
+   if (!param)
+   die("BUG: counted %d != ended at %d",
+   check->nr, cnt);
+   attr = git_attr(param);
+   if (!attr)
+   die("BUG: %s: not a valid attribute name", param);
+   check->items[cnt].attr = attr;
+   }
+   va_end(params);
+   return check;
+}
+
+struct attr_check_item *attr_check_append(struct attr_check *check,
+ const struct git_attr *attr)
+{
+   struct attr_check_item *item;
+
+   ALLOC_GROW(check->items, check->nr + 1, check->alloc);
+   item = >items[check->nr++];
+   item->attr = attr;
+   return item;
+}
+
+void attr_check_reset(struct attr_check *check)
+{
+   check->nr = 0;
+}
+
+void attr_check_clear(struct attr_check *check)
+{
+   free(check->items);
+   check->items = NULL;
+   check->alloc = 0;
+   check->nr = 0;
+}
+
+void attr_check_free(struct attr_check *check)
+{
+   attr_check_clear(check);
+   free(check);
+}
+
 static const char *builtin_attr[] = {
"[attr]binary -diff -merge -text",
NULL,
@@ -865,6 +934,11 @@ int git_all_attrs(const char *path, int *num, struct 
attr_check_item **check)
return 0;
 }
 
+int git_check_attr(const char *path, struct attr_check *check)
+{
+   return git_check_attrs(path, check->nr, check->items);
+}
+
 void git_attr_set_direction(enum git_attr_direction new, struct index_state 
*istate)
 {
enum git_attr_direction old = direction;
diff --git a/attr.h b/attr.h
index efc7bb3b3..e611b139a 100644
--- a/attr.h
+++ b/attr.h
@@ -29,6 +29,22 @@ struct attr_check_item {
const char *value;
 };
 
+struct attr_check {
+   int nr;
+  

Re: gitconfig get out of sync with submodule entries on branch switch

[Brandon Williams]

On 01/30, Benjamin Schindler wrote:
> Hi
> 
> Consider the following usecase: I have the master branch where I
> have a submodule A. I create a branch where I rename the submodule
> to be in the directory B. After doing all of this, everything looks
> good.
> Now, I switch back to master. The first oddity is, that it fails to
> remove the folder B because there are still files in there:
> 
> bschindler@metis ~/Projects/submodule_test (testbranch) $ git
> checkout master
> warning: unable to rmdir other_submodule: Directory not empty
> Switched to branch 'master'
> 
> Git submodule deinit on B fails because the submodule is not known
> to git anymore (after all, the folder B exists only in the other
> branch). I can easily just remove the folder B from disk and
> initialize the submodule A again, so all seems good.
> 
> However, what is not good is that the submodule b is still known in
> .git/config. This is in particular a problem for us, because I know
> a number of tools which use git config to retrieve the submodule
> list. Is it therefore a bug that upon branch switch, the submodule
> gets deregistered, but its entry in .git/config remains?
> 
> thanks a lot
> Benjamin Schindler
> 
> P.s. I did not subscribe to the mailing list, please add me at least
> do CC. Thanks

submodules and checkout don't really play nicely with each other at the
moment.  Stefan (cc'd) is currently working on a patch series to improve
the behavior of checkout with submodules.  Currently, if you want to
ensure you have a good working state after a checkout you should run
`git submodule update` to update all of the submoules.  As far as your
submodule still being listed in the config, that should be expected
given the scenario you described.

If I'm understanding you correctly, A and B are both the same submodule
just renamed on a different branch.  The moment you add a submoule to a
repository it is given a name which is fixed.  Typically this is the
path from the root of the repository.  The thing is, since you are able
to freely move a submodule, its path can change.  To account for this
there is the .gitmodules file which allows you to do a lookup from
submodule name to the path at which it exists (or vice versa).  The
submodules that are stored in .git/config are those which are
'initialize' or rather the submodules in which you are interested in and
will be updated by `git submodule update`.  So given your scenario you
should only have a single submodule in .git/config and the .gitmodules
file should have a single entry with a differing path for each branch.

Hopefully this gives you a bit more information to work with.  Since
Stefan has been working with this more recently than me he may have some
more input.

-- 
Brandon Williams

Re: [PATCH 5/5] graph: use SWAP macro

[René Scharfe]

Am 30.01.2017 um 17:16 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


Exchange the values of graph->columns and graph->new_columns using the
macro SWAP instead of hand-rolled code.  The result is shorter and
easier to read.

This transformation was not done by the semantic patch swap.cocci
because there's an unrelated statement between the second and the last
step of the exchange, so it didn't match the expected pattern.


Is it really true that Coccinelle cannot be told to look for a code block
that declares a variable that is then used *only* in the lines we want to
match and replace?


Hope I parsed your question correctly; my answer would be that it can't 
be true because that's basically what the proposed semantic patch does:


@ swap @
type T;
T tmp, a, b;
@@
- tmp = a;
- a = b;
- b = tmp;
+ SWAP(a, b);

@ extends swap @
identifier unused;
@@
  {
  ...
- T unused;
  ... when != unused
  }

The first part (up to the "+") looks for a opportunities to use SWAP, 
and the second part looks for blocks where that transformation was done 
and we declare identifiers that are/became unused.


It did not match the code in graph.c because the pattern was basically:

tmp = a;
a = b;
something = totally_different;
b = tmp;

Coccinelle can be told to ignore such unrelated code by adding "... when 
!= tmp" etc. (which matches context lines that don't reference tmp), but 
that's slooow.  (Perhaps I just did it wrong, though.)


René

Re: [PATCH 3/5] use SWAP macro

[René Scharfe]

Am 30.01.2017 um 17:03 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 806dd7a885..8ce00480cd 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -147,9 +147,7 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
tree1 = opt->pending.objects[0].item;
tree2 = opt->pending.objects[1].item;
if (tree2->flags & UNINTERESTING) {
-   struct object *tmp = tree2;
-   tree2 = tree1;
-   tree1 = tmp;
+   SWAP(tree2, tree1);
}


Is there a way to transform away the curly braces for blocks that become
single-line blocks, too?


Interesting question.  I guess this can be done by using a Python script 
(see contrib/coccinelle/strbuf.cocci for an example).  I'll leave this 
as homework for readers interested in Coccinelle, at least for a while. :)


René

Re: [PATCH 4/5] diff: use SWAP macro

[René Scharfe]

Am 30.01.2017 um 17:04 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


Use the macro SWAP to exchange the value of pairs of variables instead
of swapping them manually with the help of a temporary variable.  The
resulting code is shorter and easier to read.

The two cases were not transformed by the semantic patch swap.cocci
because it's extra careful and handles only cases where the types of all
variables are the same -- and here we swap two ints and use an unsigned
temporary variable for that.  Nevertheless the conversion is safe, as
the value range is preserved with and without the patch.


One way to make this more obvious would be to change the type to signed
first, and then transform (which then would catch these cases too,
right?).


I'm not sure it would be more obvious, but it would certainly make the 
type change more explicit.  In diff-index.c we might even want to change 
the type of the swapped values from int to unsigned, which is more 
fitting for file modes.  In diff.c we'd need to add a separate variable, 
as tmp is shared with other (unsigned) swaps.


René

Re: git-daemon shallow checkout fail

[Jeff King]

On Sat, Jan 28, 2017 at 05:29:32PM -0700, Bob Proulx wrote:

> However the problem driving me crazy is that this only fails this way
> on one machine.  Unfortunately failing on the machine I need to use.
> If I try this same setup on any other machine I try then there is no
> failure and it works okay.  Therefore I conclude that in the failing
> case it is trying to write a shallow_XX file in the repository but
> in all of the passing cases it does not.  I browsed through the
> git-daemon source but couldn't deduce the flow yet.
> 
> Does anyone know why one system would try to create shallow_XX
> files in the repository while another one would not?

It depends on the git version on the server. The interesting code is in
upload-pack.c, which is spawned by git-daemon to serve a fetch or clone
request.

See commit b790e0f67 (upload-pack: send shallow info over stdin to
pack-objects, 2014-03-11), which lays out the history. Since that commit
(in git v2.0.0), there should be no tmpfile needed.

> Of course git-daemon running as nobody can't create a temporary file
> shallow_XX in the /srv/git/test-project.git because it has no
> permissions by design.  But why does this work on other systems and
> not work on my target system?
> 
>   git --version  # from today's git clone build
>   git version 2.11.0.485.g4e59582

This shouldn't be happening with git v2.11. Are you sure that the "git
daemon" invocation is running that same version? I notice you set up a
restricted PATH. Is it possible that /usr/local/bin or /usr/bin has an
older version of git?

-Peff

Re: [PATCH v2] git-p4: Fix git-p4.mapUser on Windows

[Junio C Hamano]

Luke Diamand  writes:

> On 27 January 2017 at 17:33, Junio C Hamano  wrote:
>>
>> Luke, Lars, this version seems to be in line with the conclusion of
>> your earlier reviews, e.g.
>>
>> 
>>
>> Even though it looks OK to my eyes, I'll wait for Acks or further
>> refinement suggestions from either of you two before acting on this
>> patch.
>
> It looks good to me. The tests all pass, and the change looks correct.

Thanks, queued.

Re: [PATCH v2] help: improve is_executable() on Windows

[Junio C Hamano]

Johannes Schindelin  writes:

> From: Heiko Voigt 
>
> On Windows, executables need to have the file extension `.exe`, or they
> are not executables. Hence, to support scripts, Git for Windows also
> looks for a she-bang line by opening the file in question, and executing
> it via the specified script interpreter.
>
> To figure out whether files in the `PATH` are executable, `git help` has
> code that imitates this behavior. With one exception: it *always* opens
> the files and looks for a she-bang line *or* an `MZ` tell-tale
> (nevermind that files with the magic `MZ` but without file extension
> `.exe` would still not be executable).
>
> Opening this many files leads to performance problems that are even more
> serious when a virus scanner is running. Therefore, let's change the
> code to look for the file extension `.exe` early, and avoid opening the
> file altogether if we already know that it is executable.

Much more readable than the initial round.  Will queue; thanks.

Re: [PATCH v3] mingw: allow hooks to be .exe files

[Junio C Hamano]

Johannes Schindelin  writes:

> Executable files in Windows need to have the extension '.exe', otherwise
> they do not work. Extend the hooks to not just look at the hard coded
> names, but also at the names extended by the custom STRIP_EXTENSION,
> which is defined as '.exe' in Windows.

Will replace, and looks good enough for 'next'.  Let's stop
iterating and go incremental if/as needed.

Thanks.

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 30.01.2017 um 17:01 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092b..66cd466eea 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char 
*suffix)
return strip_suffix(str, suffix, );
 }

+#define SWAP(a, b) do {\
+   void *_swap_a_ptr = &(a);   \
+   void *_swap_b_ptr = &(b);   \
+   unsigned char _swap_buffer[sizeof(a)];  \
+   memcpy(_swap_buffer, _swap_a_ptr, sizeof(a));   \
+   memcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\
+  BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));   \
+   memcpy(_swap_b_ptr, _swap_buffer, sizeof(a));   \
+} while (0)
+
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)


It may seem as a matter of taste, or maybe not: I prefer this without the
_swap_a_ptr (and I would also prefer not to use identifiers starting with
an underscore, as section 7.1.3 Reserved Identifiers of the C99 standard
says they are reserved):

+#define SWAP(a, b) do {\
+   unsigned char swap_buffer_[sizeof(a)];  \
+   memcpy(swap_buffer_, &(a), sizeof(a));  \
+   memcpy(&(a), &(b), sizeof(a) +  \
+  BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));   \
+   memcpy(&(b), swap_buffer_, sizeof(a));  \
+} while (0)


We can move the underscore to the end, but using a and b directly will 
give surprising results if the parameters have side effects.  E.g. if 
you want to swap the first two elements of two arrays you might want to 
do this:


SWAP(*x++, *y++);
SWAP(*x++, *y++);

And that would increment twice as much as one would guess and access 
unexpected elements.



One idea to address the concern that not all C compilers people use to
build Git may optimize away those memcpy()s: we could also introduce a
SWAP_PRIMITIVE_TYPE (or SWAP2 or SIMPLE_SWAP or whatever) that accepts
only primitive types. But since __typeof__() is not portable...


I wouldn't worry too much about such a solution before seeing that SWAP 
(even with memcpy(3) -- this function is probably optimized quite 
heavily on most platforms) causes an actual performance problem.


René

Re: git-daemon shallow checkout fail

[Johannes Schindelin]

Hi Bob,

On Sat, 28 Jan 2017, Bob Proulx wrote:

> And the server side says:
> 
>   [26071] Request upload-pack for '/test-project.git'
>   [26071] fatal: Unable to create temporary file 
> '/srv/git/test-project.git/shallow_xKwnvZ': Permission denied
>   [26055] [26071] Disconnected (with error)

Assuming that you can rebuild your Git with debug symbols and without
optimization (simply remove the -O2 from CFLAGS in the Makefile, I never
had any luck with single-stepping in gdb when compiled with -O2), you
could attach gdb to the git-daemon and/or upload-pack process. Setting a
breakpoint on die_builtin in the failing process should give you a good
idea why things are failing, at least looking at the stacktrace.

A few more tidbits from a cursory look at the Git source code with `git
grep` and the likes:

- that error message comes from shallow.c's setup_temporary_shallow()
  function

- that function is only called from fetch-pack and receive-pack, neither
  of which should be called by upload-pack, so it is a puzzle

- adding a test case to t5570-git-daemon.sh that tests specifically your
  described scenario seems *not* to fail:

-- snip --
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 225a022e8a..0256c9aded 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -186,5 +186,17 @@ test_expect_success 'hostname cannot break out of 
directory' '
git clone --bare "$GIT_DAEMON_URL/escape.git" tmp.git
 '
 
+test_expect_success POSIXPERM 'shallow clone from read-only server' '
+   test_when_finished "rm -rf tmp.git" &&
+   repo="$GIT_DAEMON_DOCUMENT_ROOT_PATH/readonly.git" &&
+   git init --bare "$repo" &&
+   git push "$repo" HEAD &&
+   >"$repo"/git-daemon-export-ok &&
+   chmod a-w "$repo" &&
+   test_must_fail \
+   env GIT_OVERRIDE_VIRTUAL_HOST=.. \
+   git clone --depth 1 "$GIT_DAEMON_URL/readonly.git" tmp.git
+'
+
 stop_git_daemon
 test_done
-- snap --

- I even modified t/lib-git-daemon.sh to start the daemon as `nobody` and
  kill it as `root`, and I won't share that patch because it is as
  ugly, but *even then* the test succeeded.

So my suspicion is that the repository you try to serve may already be
shallow, or something else funky is going on that has not been included in
your report.

The most direct way to get to the bottom of this may be to do something
like this:

-- snip --
diff --git a/shallow.c b/shallow.c
index 11f7dde9d9..30f5c96d50 100644
--- a/shallow.c
+++ b/shallow.c
@@ -288,12 +288,18 @@ int write_shallow_commits(struct strbuf *out, int 
use_pack_protocol,
 
 static struct tempfile temporary_shallow;
 
+static int debug_me;
+
 const char *setup_temporary_shallow(const struct sha1_array *extra)
 {
struct strbuf sb = STRBUF_INIT;
int fd;
 
if (write_shallow_commits(, 0, extra)) {
+error("About to create shallow_XX: pid = %d", getpid());
+while (!debug_me) {
+   sleep(1);
+}
fd = xmks_tempfile(_shallow, 
git_path("shallow_XX"));
 
if (write_in_full(fd, sb.buf, sb.len) != sb.len)
-- snap --

Then let it run, wait for the error message "About to create
shallow_XX" and then attach with a gdb started as nobody via `attach
` to see the stack trace.

That should give you an idea where that code path is hit (unexpectedly).

Ciao,
Johannes

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 30.01.2017 um 16:39 schrieb Johannes Schindelin:

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:


Add a macro for exchanging the values of variables.  It allows users to
avoid repetition and takes care of the temporary variable for them.  It
also makes sure that the storage sizes of its two parameters are the
same.  Its memcpy(1) calls are optimized away by current compilers.


How certain are we that "current compilers" optimize that away? And about
which "current compilers" are we talking? GCC? GCC 6? Clang? Clang 3?
Clang 3.8.*? Visual C 2005+?


GCC 4.4.7 and clang 3.2 on x86-64 compile the macro to the same object 
code as a manual swap ; see https://godbolt.org/g/F4b9M9.  Both were 
released 2012.  That website doesn't offer Visual C(++), but since you 
added the original macro in e5a94313c0 ("Teach git-apply about '-R'", 
2006) I guess we have that part covered. ;)


René

Re: [PATCH 5/5] graph: use SWAP macro

[Johannes Schindelin]

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:

> Exchange the values of graph->columns and graph->new_columns using the
> macro SWAP instead of hand-rolled code.  The result is shorter and
> easier to read.
> 
> This transformation was not done by the semantic patch swap.cocci
> because there's an unrelated statement between the second and the last
> step of the exchange, so it didn't match the expected pattern.

Is it really true that Coccinelle cannot be told to look for a code block
that declares a variable that is then used *only* in the lines we want to
match and replace?

I never used the tool, and a quick web search did not clarify the picture,
either...

Ciao,
Dscho

gitconfig get out of sync with submodule entries on branch switch

[Benjamin Schindler]

Hi

Consider the following usecase: I have the master branch where I have a 
submodule A. I create a branch where I rename the submodule to be in the 
directory B. After doing all of this, everything looks good.
Now, I switch back to master. The first oddity is, that it fails to 
remove the folder B because there are still files in there:


bschindler@metis ~/Projects/submodule_test (testbranch) $ git checkout 
master

warning: unable to rmdir other_submodule: Directory not empty
Switched to branch 'master'

Git submodule deinit on B fails because the submodule is not known to 
git anymore (after all, the folder B exists only in the other branch). I 
can easily just remove the folder B from disk and initialize the 
submodule A again, so all seems good.


However, what is not good is that the submodule b is still known in 
.git/config. This is in particular a problem for us, because I know a 
number of tools which use git config to retrieve the submodule list. Is 
it therefore a bug that upon branch switch, the submodule gets 
deregistered, but its entry in .git/config remains?


thanks a lot
Benjamin Schindler

P.s. I did not subscribe to the mailing list, please add me at least do 
CC. Thanks

Re: [PATCH 4/5] diff: use SWAP macro

[Johannes Schindelin]

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:

> Use the macro SWAP to exchange the value of pairs of variables instead
> of swapping them manually with the help of a temporary variable.  The
> resulting code is shorter and easier to read.
> 
> The two cases were not transformed by the semantic patch swap.cocci
> because it's extra careful and handles only cases where the types of all
> variables are the same -- and here we swap two ints and use an unsigned
> temporary variable for that.  Nevertheless the conversion is safe, as
> the value range is preserved with and without the patch.

One way to make this more obvious would be to change the type to signed
first, and then transform (which then would catch these cases too,
right?).

Ciao,
Dscho

Re: [PATCH 3/5] use SWAP macro

[Johannes Schindelin]

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:

> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> index 806dd7a885..8ce00480cd 100644
> --- a/builtin/diff-tree.c
> +++ b/builtin/diff-tree.c
> @@ -147,9 +147,7 @@ int cmd_diff_tree(int argc, const char **argv, const char 
> *prefix)
>   tree1 = opt->pending.objects[0].item;
>   tree2 = opt->pending.objects[1].item;
>   if (tree2->flags & UNINTERESTING) {
> - struct object *tmp = tree2;
> - tree2 = tree1;
> - tree1 = tmp;
> + SWAP(tree2, tree1);
>   }

Is there a way to transform away the curly braces for blocks that become
single-line blocks, too?

Ciao,
Dscho

Re: [PATCH 1/5] add SWAP macro

[Johannes Schindelin]

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 87237b092b..66cd466eea 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char 
> *suffix)
>   return strip_suffix(str, suffix, );
>  }
>  
> +#define SWAP(a, b) do {  \
> + void *_swap_a_ptr = &(a);   \
> + void *_swap_b_ptr = &(b);   \
> + unsigned char _swap_buffer[sizeof(a)];  \
> + memcpy(_swap_buffer, _swap_a_ptr, sizeof(a));   \
> + memcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\
> +BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));   \
> + memcpy(_swap_b_ptr, _swap_buffer, sizeof(a));   \
> +} while (0)
> +
>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)

It may seem as a matter of taste, or maybe not: I prefer this without the
_swap_a_ptr (and I would also prefer not to use identifiers starting with
an underscore, as section 7.1.3 Reserved Identifiers of the C99 standard
says they are reserved):

+#define SWAP(a, b) do {\
+   unsigned char swap_buffer_[sizeof(a)];  \
+   memcpy(swap_buffer_, &(a), sizeof(a));  \
+   memcpy(&(a), &(b), sizeof(a) +  \
+  BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));   \
+   memcpy(&(b), swap_buffer_, sizeof(a));  \
+} while (0)

One idea to address the concern that not all C compilers people use to
build Git may optimize away those memcpy()s: we could also introduce a
SWAP_PRIMITIVE_TYPE (or SWAP2 or SIMPLE_SWAP or whatever) that accepts
only primitive types. But since __typeof__() is not portable...

Ciao,
Dscho

Re: [PATCH 1/5] add SWAP macro

[Johannes Schindelin]

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:

> Add a macro for exchanging the values of variables.  It allows users to
> avoid repetition and takes care of the temporary variable for them.  It
> also makes sure that the storage sizes of its two parameters are the
> same.  Its memcpy(1) calls are optimized away by current compilers.

How certain are we that "current compilers" optimize that away? And about
which "current compilers" are we talking? GCC? GCC 6? Clang? Clang 3?
Clang 3.8.*? Visual C 2005+?

Ciao,
Dscho

Re: [PATCH v4] t/Makefile: add a rule to re-run previously-failed tests

[Johannes Schindelin]

Hi Junio,

On Fri, 27 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > This patch automates the process of determining which tests failed
> > previously and re-running them.
> > ...
> >
> > Signed-off-by: Johannes Schindelin 
> 
> I stored both versions in files and compared them, and it seems the
> single word change in the proposed commit log message is the only
> difference.  I would have written "Automate the process...", though.

Yes, we have different styles. Thanks for letting my commit keep my commit
message this time ;-)

> If you are resending, touching up to cover all points raised by a
> reviewer and doing nothing else, having "Reviewed-by: Jeff King
> " would have been nicer.

TBH I am not at all sure that I know when to add those footers and when
not. After having been asked to remove such a footer, I decided to *not*
include them by default.

Having gray zones about the footers strikes me as similar to having gray
zones in the coding style guidelines: it sure gives the contributors more
freedom, but it also creates uncertainty and as a consequence takes up a
lot of reviewing space and time (hence taking away space and time from
reviewing the code for bugs).

In other words: while I appreciate the idea of giving contributors such as
myself a lot of leeway, I would love even more to be able to automate away
tedious and boring tasks (such as adding Tested-by: or Reviewed-by:
footers, or for that matter, addressing code style issues before any
reviewer has to shed bikes so that they can focus on the parts of the
review that no machine can do for them).

Ciao,
Johannes

Re: [PATCH] help: correct behavior for is_executable on Windows

[Johannes Schindelin]

Hi Junio,

On Fri, 27 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > From: Heiko Voigt 
> >
> > The previous implementation said that the filesystem information on
> > Windows is not reliable to determine whether a file is executable. To
> > gather this information it was peeking into the first two bytes of a
> > file to see whether it looks executable.
> >
> > Apart from the fact that on Windows executables are defined as such by
> > their extension (and Git has special code to support "executing"
> > scripts when they have a she-bang line) it leads to serious
> > performance problems: not only do we have to open many files now, it
> > gets even slower when a virus scanner is running.
> 
> Heiko, around here (before going into the details of how severe the
> problem is and how wonderful the result applying of this change is) is
> the best place to summarize the solution.  E.g.
> 
>   Because the definition of "executable" on Windows is based
>   on the file extension, update the function to declare that a
>   file with ".exe" extension without opening and reading the
>   early bytes from it.  This avoids serious performance issues.
> 
> I paraphrased the rest only so that the description of the solution
> (i.e. "instead of opening and peeking, we trust .exe suffix") fits well
> in the surrounding text; the important part is to say what the change
> does clearly.

I adjusted the commit message. It was tweaked a little differently from
what you suggested, as I preferred to condense the information a bit more.

> I agree with the reasoning and the execution of the patch, except
> that 
> 
>  - "correct behaviour" in the title makes it appear that this is a
>correctness thing, but this is primarily a performance fix.

Primarily. But not only. The magic `MZ` without the file extension `.exe`
is pretty useless, as the file could not be executed, still.

To avoid further turnaround, though, I also edited the contentious
"correct" to read "improve" now.

>  - It is a bit strange that "MZ" is dropped in the same patch
>without any mention.

I fixed that in the commit message.

Ciao,
Johannes

Re: [PATCH] checkout: convert post_checkout_hook() to struct object_id

[Johannes Schindelin]

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:

> Signed-off-by: Rene Scharfe 

These three SHA-1 -> OID patches all appear correct to me.

Ciao,
Dscho

[PATCH v2] help: improve is_executable() on Windows

[Johannes Schindelin]

From: Heiko Voigt 

On Windows, executables need to have the file extension `.exe`, or they
are not executables. Hence, to support scripts, Git for Windows also
looks for a she-bang line by opening the file in question, and executing
it via the specified script interpreter.

To figure out whether files in the `PATH` are executable, `git help` has
code that imitates this behavior. With one exception: it *always* opens
the files and looks for a she-bang line *or* an `MZ` tell-tale
(nevermind that files with the magic `MZ` but without file extension
`.exe` would still not be executable).

Opening this many files leads to performance problems that are even more
serious when a virus scanner is running. Therefore, let's change the
code to look for the file extension `.exe` early, and avoid opening the
file altogether if we already know that it is executable.

See the following measurements (in seconds) as an example, where we
execute a simple program that simply lists the directory contents and
calls open() on every listed file:

With virus scanner running (coldcache):

$ ./a.exe /libexec/git-core/
before open (git-add.exe): 0.00
after open (git-add.exe): 0.412873
before open (git-annotate.exe): 0.000175
after open (git-annotate.exe): 0.397925
before open (git-apply.exe): 0.000243
after open (git-apply.exe): 0.36
before open (git-archive.exe): 0.000147
after open (git-archive.exe): 0.397783
before open (git-bisect--helper.exe): 0.000160
after open (git-bisect--helper.exe): 0.397700
before open (git-blame.exe): 0.000160
after open (git-blame.exe): 0.399136
...

With virus scanner running (hotcache):

$ ./a.exe /libexec/git-core/
before open (git-add.exe): 0.00
after open (git-add.exe): 0.000325
before open (git-annotate.exe): 0.000229
after open (git-annotate.exe): 0.000177
before open (git-apply.exe): 0.000167
after open (git-apply.exe): 0.000150
before open (git-archive.exe): 0.000154
after open (git-archive.exe): 0.000156
before open (git-bisect--helper.exe): 0.000132
after open (git-bisect--helper.exe): 0.000180
before open (git-blame.exe): 0.000718
after open (git-blame.exe): 0.000724
...

With this patch I get:

$ time git help git
Launching default browser to display HTML ...

real0m8.723s
user0m0.000s
sys 0m0.000s

and without

$ time git help git
Launching default browser to display HTML ...

real1m37.734s
user0m0.000s
sys 0m0.031s

both tests with cold cache and giving the machine some time to settle
down after restart.

[jes: adjusted the commit message]

Signed-off-by: Heiko Voigt 
Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/help-is-exe-v2
Fetch-It-Via: git fetch https://github.com/dscho/git help-is-exe-v2

 help.c | 21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/help.c b/help.c
index 53e2a67e00..bc6cd19cf3 100644
--- a/help.c
+++ b/help.c
@@ -105,7 +105,22 @@ static int is_executable(const char *name)
return 0;
 
 #if defined(GIT_WINDOWS_NATIVE)
-{  /* cannot trust the executable bit, peek into the file instead */
+   /*
+* On Windows there is no executable bit. The file extension
+* indicates whether it can be run as an executable, and Git
+* has special-handling to detect scripts and launch them
+* through the indicated script interpreter. We test for the
+* file extension first because virus scanners may make
+* it quite expensive to open many files.
+*/
+   if (ends_with(name, ".exe"))
+   return S_IXUSR;
+
+{
+   /*
+* Now that we know it does not have an executable extension,
+* peek into the file instead.
+*/
char buf[3] = { 0 };
int n;
int fd = open(name, O_RDONLY);
@@ -113,8 +128,8 @@ static int is_executable(const char *name)
if (fd >= 0) {
n = read(fd, buf, 2);
if (n == 2)
-   /* DOS executables start with "MZ" */
-   if (!strcmp(buf, "#!") || !strcmp(buf, "MZ"))
+   /* look for a she-bang */
+   if (!strcmp(buf, "#!"))
st.st_mode |= S_IXUSR;
close(fd);
}

base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
-- 
2.11.1.windows.prerelease.2.9.g3014b57

Re: [PATCH] fixup! worktree move: new command

[Johannes Schindelin]

Hi Junio,

On Fri, 27 Jan 2017, Junio C Hamano wrote:

> The tip of 'pu' (or anything beyond the tip of 'jch') is not always
> expected to pass test or even build, [...]

This makes `pu` a lot less useful than it could be.

And we could easily improve the situation simply by changing the rule ever
so slightly: when a build, or a test, fails in `pu` and there exists a
fix, this fix should go into `pu` ASAP.

As you point out later in your mail, the fixup! or SQUASH! commit is a
very convenient reminder that a particular branch is still "under
construction". That is, changing the rule as I proposed above will not
only help the Continuous Integration [*1*] to avoid reporting duplicates,
it will also help us improve the project faster.

Ciao,
Johannes

Footnote *1*: It appears that there may be the misconception floating
around that Continuous Integration is designed to annoy developers by
pointing out unportable or unbuildable code. Once you realize, though,
that it detects and reports code that is below our existing code's
quality, no doubt you will agree that it is a convenient tool to relieve
reviewers from tedious work that can be done by a machine as well.

Re: [PATCH] mingw: allow hooks to be .exe files

[Johannes Schindelin]

Hi Stefan,

On Fri, 27 Jan 2017, Stefan Beller wrote:

> On Fri, Jan 27, 2017 at 2:29 AM, Johannes Schindelin
>  wrote:
> > Hi Junio,
> >
> > On Thu, 26 Jan 2017, Junio C Hamano wrote:
> >
> >> Johannes Schindelin  writes:
> >>
> >> > On Wed, 25 Jan 2017, Jeff King wrote:
> >> >
> >> > v2 coming,
> 
> The commit message, while technically correct, seems a bit off. This is
> because the commit message only talks about .exe extensions, but the
> change in code doesn't even have the string "exe" mentioned once.
> 
> With this discussion here, it is easy for me to connect the dots, but it
> would be nice to have the full picture in the commit message.

I just sent out v3, using a slightly tweaked version of the commit message
you proposed.

Ciao,
Dscho

[PATCH v3] mingw: allow hooks to be .exe files

[Johannes Schindelin]

Executable files in Windows need to have the extension '.exe', otherwise
they do not work. Extend the hooks to not just look at the hard coded
names, but also at the names extended by the custom STRIP_EXTENSION,
which is defined as '.exe' in Windows.

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/exe-as-hook-v3
Fetch-It-Via: git fetch https://github.com/dscho/git exe-as-hook-v3

 run-command.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 73bfba7ef9..5227f78aea 100644
--- a/run-command.c
+++ b/run-command.c
@@ -871,8 +871,14 @@ const char *find_hook(const char *name)
 
strbuf_reset();
strbuf_git_path(, "hooks/%s", name);
-   if (access(path.buf, X_OK) < 0)
+   if (access(path.buf, X_OK) < 0) {
+#ifdef STRIP_EXTENSION
+   strbuf_addstr(, STRIP_EXTENSION);
+   if (access(path.buf, X_OK) >= 0)
+   return path.buf;
+#endif
return NULL;
+   }
return path.buf;
 }
 

base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
-- 
2.11.1.windows.prerelease.2.9.g3014b57

Hello.

[Richard Fennell]

I Richard Fennell, From Bendigo Bank Australia and I am writing this letter to 
submit an interesting innovating business idea that I would like to share with 
you, the business is real and risk free, it shall be done in accordance with 
the appropriate banking regulation and finance policy.
You will be entitled to thirty percent reimbursement from USD1400.00, I 
look forward to hearing from you and appreciate your time. Thank you in advance.

Sincerely,
Richard Fennell
Bendigo and Adelaide Bank Australia |