* Re: [PATCH 1/5] add SWAP macro
From: Johannes Schindelin @ 2017-01-30 21:03 UTC (permalink / raw)
To: Johannes Sixt; +Cc: René Scharfe, Git List, Junio C Hamano
In-Reply-To: <aa653d57-4a97-ac50-b20c-f94ed43a22fb@kdbg.org>
[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]
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, &len);
> > > }
> > >
> > > +#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
^ permalink raw reply
* Re: difflame
From: Junio C Hamano @ 2017-01-30 21:08 UTC (permalink / raw)
To: Jeff King; +Cc: Edmundo Carmona Antoranz, Git List
In-Reply-To: <20170128035321.yrcqwkg2fiwadxj4@sigill.intra.peff.net>
Jeff King <peff@peff.net> 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...
^ permalink raw reply
* Re: [RFC] Proof of concept: Support multiple authors
From: Junio C Hamano @ 2017-01-30 20:48 UTC (permalink / raw)
To: Christian Couder; +Cc: Cornelius Schumacher, git, Josh Triplett
In-Reply-To: <CAP8UFD3=vaFupEDay-5vrMBwK_YJezysUUvySxnUUZxuW7m_WQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> 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_HL7XGnxZrVu3oSnsbnTyxbg8Vh6vzi4c1isSrrexYQ@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-wsa@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.
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Johannes Schindelin @ 2017-01-30 20:48 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <9bcae427-d654-a671-4368-030150168102@web.de>
[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]
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:
00007FF791311000 mov eax,dword ptr [rcx]
00007FF791311002 mov r8d,dword ptr [rdx]
00007FF791311005 mov dword ptr [rcx],r8d
00007FF791311008 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
^ permalink raw reply
* [PATCH 1/4] git-prompt.sh: add submodule indicator
From: Benjamin Fuchs @ 2017-01-30 20:44 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sbeller, sandals, ville.skytta, Benjamin Fuchs
In-Reply-To: <1485809065-11978-1-git-send-email-email@benjaminfuchs.de>
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 <email@benjaminfuchs.de>
---
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
^ permalink raw reply related
* [PATCH 2/4] git-prompt.sh: rework of submodule indicator
From: Benjamin Fuchs @ 2017-01-30 20:44 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sbeller, sandals, ville.skytta, Benjamin Fuchs
In-Reply-To: <1485809065-11978-1-git-send-email-email@benjaminfuchs.de>
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 <email@benjaminfuchs.de>
---
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
^ permalink raw reply related
* [PATCH 4/4] git-prompt.sh: add tests for submodule indicator
From: Benjamin Fuchs @ 2017-01-30 20:44 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sbeller, sandals, ville.skytta, Benjamin Fuchs
In-Reply-To: <1485809065-11978-1-git-send-email-email@benjaminfuchs.de>
Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
---
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
^ permalink raw reply related
* [PATCH 0/4] git-prompt.sh: Full patch for submodule indicator
From: Benjamin Fuchs @ 2017-01-30 20:44 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sbeller, sandals, ville.skytta, 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
^ permalink raw reply
* [PATCH 3/4] git-prompt.sh: fix for submodule 'dirty' indicator
From: Benjamin Fuchs @ 2017-01-30 20:44 UTC (permalink / raw)
To: git; +Cc: szeder.dev, sbeller, sandals, ville.skytta, Benjamin Fuchs
In-Reply-To: <1485809065-11978-1-git-send-email-email@benjaminfuchs.de>
Fixing wrong git diff line.
Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
---
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
^ permalink raw reply related
* Re: [RFC] Proof of concept: Support multiple authors
From: Cornelius Schumacher @ 2017-01-30 19:33 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Josh Triplett
In-Reply-To: <CAP8UFD3=vaFupEDay-5vrMBwK_YJezysUUvySxnUUZxuW7m_WQ@mail.gmail.com>
On Monday 30 January 2017 18:56:42 Christian Couder wrote:
> On Sun, Jan 29, 2017 at 7:06 PM, Cornelius Schumacher
> <schumacher@kde.org> 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
> exYQ@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 <schumacher@kde.org>
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: Johannes Sixt @ 2017-01-30 18:41 UTC (permalink / raw)
To: Johannes Schindelin, René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301643260.3469@virtualbox>
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, &len);
>> }
>>
>> +#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
^ permalink raw reply
* Re: [RFC] Proof of concept: Support multiple authors
From: Christian Couder @ 2017-01-30 17:56 UTC (permalink / raw)
To: Cornelius Schumacher; +Cc: git, Josh Triplett
In-Reply-To: <1485713194-11782-1-git-send-email-schumacher@kde.org>
Hi,
On Sun, Jan 29, 2017 at 7:06 PM, Cornelius Schumacher
<schumacher@kde.org> 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_HL7XGnxZrVu3oSnsbnTyxbg8Vh6vzi4c1isSrrexYQ@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 <schumacher@kde.org>
> ---
>
> 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.
^ permalink raw reply
* [PATCH v3 15/27] attr: (re)introduce git_check_attr() and struct attr_check
From: Brandon Williams @ 2017-01-30 18:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Brandon Williams
In-Reply-To: <20170128020207.179015-16-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
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 = &check->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;
+ int alloc;
+ struct attr_check_item *items;
+};
+
+extern struct attr_check *attr_check_alloc(void);
+extern struct attr_check *attr_check_initl(const char *, ...);
+
+extern struct attr_check_item *attr_check_append(struct attr_check *check,
+ const struct git_attr *attr);
+
+extern void attr_check_reset(struct attr_check *check);
+extern void attr_check_clear(struct attr_check *check);
+extern void attr_check_free(struct attr_check *check);
+
/*
* Return the name of the attribute represented by the argument. The
* return value is a pointer to a null-delimited string that is part
@@ -37,6 +53,7 @@ struct attr_check_item {
extern const char *git_attr_name(const struct git_attr *);
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
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v3 16/27] attr: convert git_all_attrs() to use "struct attr_check"
From: Brandon Williams @ 2017-01-30 18:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Stefan Beller, Brandon Williams
In-Reply-To: <20170128020207.179015-17-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
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: %s\n",
+ git_attr_name(check->items[j].attr), value);
}
-
}
}
static void check_attr(const char *prefix,
- int cnt, struct attr_check_item *check,
+ struct attr_check *check,
+ int collect_all,
const char *file)
{
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
- if (check != NULL) {
- if (git_check_attrs(full_path, cnt, check))
- die("git_check_attrs died");
- output_attr(cnt, check, file);
+
+ if (collect_all) {
+ git_all_attrs(full_path, check);
} else {
- if (git_all_attrs(full_path, &cnt, &check))
- die("git_all_attrs died");
- output_attr(cnt, check, file);
- free(check);
+ if (git_check_attr(full_path, check))
+ die("git_check_attr died");
}
+ output_attr(check, file);
+
free(full_path);
}
static void check_attr_stdin_paths(const char *prefix,
- int cnt, struct attr_check_item *check)
+ struct attr_check *check,
+ int collect_all)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf unquoted = STRBUF_INIT;
@@ -85,7 +88,7 @@ static void check_attr_stdin_paths(const char *prefix,
die("line is badly quoted");
strbuf_swap(&buf, &unquoted);
}
- check_attr(prefix, cnt, check, buf.buf);
+ check_attr(prefix, check, collect_all, buf.buf);
maybe_flush_or_die(stdout, "attribute to stdout");
}
strbuf_release(&buf);
@@ -100,7 +103,7 @@ static NORETURN void error_with_usage(const char *msg)
int cmd_check_attr(int argc, const char **argv, const char *prefix)
{
- struct attr_check_item *check;
+ struct attr_check *check;
int cnt, i, doubledash, filei;
if (!is_bare_repository())
@@ -160,28 +163,25 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
error_with_usage("No file specified");
}
- if (all_attrs) {
- check = NULL;
- } else {
- check = xcalloc(cnt, sizeof(*check));
+ check = attr_check_alloc();
+ if (!all_attrs) {
for (i = 0; i < cnt; i++) {
- const char *name;
- struct git_attr *a;
- name = argv[i];
- a = git_attr(name);
+ struct git_attr *a = git_attr(argv[i]);
if (!a)
return error("%s: not a valid attribute name",
- name);
- check[i].attr = a;
+ argv[i]);
+ attr_check_append(check, a);
}
}
if (stdin_paths)
- check_attr_stdin_paths(prefix, cnt, check);
+ check_attr_stdin_paths(prefix, check, all_attrs);
else {
for (i = filei; i < argc; i++)
- check_attr(prefix, cnt, check, argv[i]);
+ check_attr(prefix, check, all_attrs, argv[i]);
maybe_flush_or_die(stdout, "attribute to stdout");
}
+
+ attr_check_free(check);
return 0;
}
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: gitconfig get out of sync with submodule entries on branch switch
From: Brandon Williams @ 2017-01-30 17:51 UTC (permalink / raw)
To: Benjamin Schindler; +Cc: git, sbeller
In-Reply-To: <0f14df64-1aa2-e671-9785-4e5e0a076ae6@gmail.com>
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
^ permalink raw reply
* Re: [PATCH 5/5] graph: use SWAP macro
From: René Scharfe @ 2017-01-30 17:41 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301714450.3469@virtualbox>
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é
^ permalink raw reply
* Re: [PATCH 3/5] use SWAP macro
From: René Scharfe @ 2017-01-30 17:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301702120.3469@virtualbox>
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é
^ permalink raw reply
* Re: git-daemon shallow checkout fail
From: Jeff King @ 2017-01-30 17:27 UTC (permalink / raw)
To: git
In-Reply-To: <20170129002932.GA19359@dismay.proulx.com>
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_XXXXXX 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_XXXXXX
> 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_XXXXXX 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
^ permalink raw reply
* Re: [PATCH 4/5] diff: use SWAP macro
From: René Scharfe @ 2017-01-30 17:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301704110.3469@virtualbox>
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é
^ permalink raw reply
* Re: [PATCH v2] git-p4: Fix git-p4.mapUser on Windows
From: Junio C Hamano @ 2017-01-30 17:07 UTC (permalink / raw)
To: Luke Diamand; +Cc: Lars Schneider, Git Users, George Vanburgh
In-Reply-To: <CAE5ih7-qug9n-Df2gA27iTjSQo67tAnPhTJWQhyvR_PP9h3rcg@mail.gmail.com>
Luke Diamand <luke@diamand.org> writes:
> On 27 January 2017 at 17:33, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Luke, Lars, this version seems to be in line with the conclusion of
>> your earlier reviews, e.g.
>>
>> <CAE5ih7_+Vc9oqKdjo8h2vgZPup4pto9wd=sBb=W6hCs4tuW2Jg@mail.gmail.com>
>>
>> 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.
^ permalink raw reply
* Re: [PATCH v2] help: improve is_executable() on Windows
From: Junio C Hamano @ 2017-01-30 17:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Heiko Voigt
In-Reply-To: <4b93fe44ff9020ed80e4fd93a24a6ffa647e7678.1485780050.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> From: Heiko Voigt <hvoigt@hvoigt.net>
>
> 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.
^ permalink raw reply
* Re: [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-01-30 16:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301643260.3469@virtualbox>
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, &len);
>> }
>>
>> +#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é
^ permalink raw reply
* Re: [PATCH v3] mingw: allow hooks to be .exe files
From: Junio C Hamano @ 2017-01-30 16:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King, Stefan Beller
In-Reply-To: <78a73c9d0a8e38fcc61302d0495533dcc4fab076.1485779272.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> 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.
^ permalink raw reply
* Re: git-daemon shallow checkout fail
From: Johannes Schindelin @ 2017-01-30 16:52 UTC (permalink / raw)
To: Bob Proulx; +Cc: git
In-Reply-To: <20170129002932.GA19359@dismay.proulx.com>
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(&sb, 0, extra)) {
+error("About to create shallow_XXXXXX: pid = %d", getpid());
+while (!debug_me) {
+ sleep(1);
+}
fd = xmks_tempfile(&temporary_shallow, git_path("shallow_XXXXXX"));
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_XXXXXX" and then attach with a gdb started as nobody via `attach
<pid>` to see the stack trace.
That should give you an idea where that code path is hit (unexpectedly).
Ciao,
Johannes
^ permalink raw reply related
* Re: [PATCH 1/5] add SWAP macro
From: René Scharfe @ 2017-01-30 16:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701301637570.3469@virtualbox>
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é
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox