* [BUG?] git log with --follow and --skip
From: Christian Couder @ 2016-10-06 20:24 UTC (permalink / raw)
To: git
Hi,
At GitLab, we are annoyed by the fact that `git log` doesn't seem to
work well when using it with both --follow and --skip.
For example:
> git log --oneline -6 README.md
a299e3a README.md: format CLI commands with code syntax
c9a014e README.md: don't take 'commandname' literally
a217f07 README.md: move down historical explanation about the name
28513c4 README.md: don't call git stupid in the title
d9b297d README.md: move the link to git-scm.com up
6164972 README.md: add hyperlinks on filenames
> git log --skip 2 --oneline -6 README.md
a217f07 README.md: move down historical explanation about the name
28513c4 README.md: don't call git stupid in the title
d9b297d README.md: move the link to git-scm.com up
6164972 README.md: add hyperlinks on filenames
4ad21f5 README: use markdown syntax
The above 2 commands seem to work well, but:
> git log --follow --skip 2 --oneline -6 README.md
a299e3a README.md: format CLI commands with code syntax
c9a014e README.md: don't take 'commandname' literally
a217f07 README.md: move down historical explanation about the name
28513c4 README.md: don't call git stupid in the title
d9b297d README.md: move the link to git-scm.com up
6164972 README.md: add hyperlinks on filenames
So with --follow it looks like the top 2 commits are not skipped any more.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH 1/2] submodule add: extend force flag to add existing repos
From: Junio C Hamano @ 2016-10-06 20:11 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, hvoigt, torvalds, peff
In-Reply-To: <20161006193725.31553-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Currently the force flag in `git submodule add` takes care of possibly
> ignored files or when a name collision occurs.
>
> However there is another situation where submodule add comes in handy:
> When you already have a gitlink recorded, but no configuration was
> done (i.e. no .gitmodules file nor any entry in .git/config) and you
> want to generate these config entries. For this situation allow
> `git submodule add` to proceed if there is already a submodule at the
> given path in the index.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
Yup, the goal makes perfect sense.
I vaguely recall discussing this exact issue of "git submodule add"
that refuses to add a path that already is a gitlink (via "git add"
that has previously been run) elsewhere on this list some time ago.
^ permalink raw reply
* Re: [PATCH v2 3/3] gitweb: Link to "git describe"'d commits in log messages
From: Junio C Hamano @ 2016-10-06 19:51 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Jakub Narebski
In-Reply-To: <20161006091135.29590-4-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Change the log formatting function to know about "git describe" output
> such as "v2.8.0-4-g867ad08", in addition to just plain "867ad08".
>
> There are still many valid refnames that we don't link to
> e.g. v2.10.0-rc1~2^2~1 is also a valid way to refer to
> v2.8.0-4-g867ad08, but I'm not supporting that with this commit,
> similarly it's trivially possible to create some refnames like
> "æ/var-gf6727b0" or which won't be picked up by this regex.
Not a serious counter-proposal or suggestion, and certainly not an
objection to the patch I am responding to, but I wonder if it is so
bad if we made the 867ad08 into link when showing v2.8.0-4-g867ad08.
IOW, in addition to \b followed by [0-9a-f]+ followed by \b, if we
allowed an optional 'g' in front of the hex, e.g.
- $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
+ $line =~ s{\bg?([0-9a-fA-F]{7,40})\b}{
wouldn't that be much simpler, covers more cases and sufficient?
> There's surely room for improvement here, but I just wanted to address
> the very common case of sticking "git describe" output into commit
> messages without trying to link to all possible refnames, that's going
> to be a rather futile exercise given that this is free text, and it
> would be prohibitively expensive to look up whether the references in
> question exist in our repository.
>
> There was on-list discussion about how we could do better than this
> patch. Junio suggested to update parse_commits() to call a new
> "gitweb--helper" command which would pass each of the revision
> candidates through "rev-parse --verify --quiet". That would cut down
> on our false positives (e.g. we'll link to "deadbeef"), and also allow
> us to be more aggressive in selecting candidate revisions.
>
> That may be too expensive to work in practice, or it may
> not. Investigating that would be a good follow-up to this patch.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> gitweb/gitweb.perl | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 92b5e91..7cf68f0 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2036,10 +2036,24 @@ sub format_log_line_html {
> my $line = shift;
>
> $line = esc_html($line, -nbsp=>1);
> - $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
> + $line =~ s{
> + \b
> + (
> + # The output of "git describe", e.g. v2.10.0-297-gf6727b0
> + # or hadoop-20160921-113441-20-g094fb7d
> + (?<!-) # see strbuf_check_tag_ref(). Tags can't start with -
> + [A-Za-z0-9.-]+
> + (?!\.) # refs can't end with ".", see check_refname_format()
> + -g[0-9a-fA-F]{7,40}
> + |
> + # Just a normal looking Git SHA1
> + [0-9a-fA-F]{7,40}
> + )
> + \b
> + }{
> $cgi->a({-href => href(action=>"object", hash=>$1),
> -class => "text"}, $1);
> - }eg;
> + }egx;
>
> return $line;
> }
^ permalink raw reply
* Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+
From: Junio C Hamano @ 2016-10-06 19:44 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Jakub Narebski
In-Reply-To: <20161006091135.29590-3-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Change the minimum length of an abbreviated object identifier in the
> commit message gitweb tries to turn into link from 8 hexchars to 7.
>
> This arbitrary minimum length of 8 was introduced in bfe2191 ("gitweb:
> SHA-1 in commit log message links to "object" view", 2006-12-10), but
> the default abbreviation length is 7, and has been for a long time.
>
> It's still possible to reference SHA1s down to 4 characters in length,
> see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make
> git actually produce that, so I doubt anyone is putting that into log
> messages in practice, but people definitely do put 7 character SHA1s
> into log messages.
>
> I think it's fairly dubious to link to things matching [0-9a-fA-F]
> here as opposed to just [0-9a-f], that dates back to the initial
> version of gitweb from 161332a ("first working version",
> 2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
> them as far as I can tell.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
s/SHA1/SHA-1/g. I agree that A-F are dubious.
> gitweb/gitweb.perl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index cba7405..92b5e91 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2036,7 +2036,7 @@ sub format_log_line_html {
> my $line = shift;
>
> $line = esc_html($line, -nbsp=>1);
> - $line =~ s{\b([0-9a-fA-F]{8,40})\b}{
> + $line =~ s{\b([0-9a-fA-F]{7,40})\b}{
> $cgi->a({-href => href(action=>"object", hash=>$1),
> -class => "text"}, $1);
> }eg;
^ permalink raw reply
* [PATCH v2 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks
From: Jeff King @ 2016-10-06 19:41 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Michael Haggerty
In-Reply-To: <1c39b371-eb41-05d9-3c48-bd41764c9c9a@kdbg.org>
On Thu, Oct 06, 2016 at 09:31:22PM +0200, Johannes Sixt wrote:
> Am 06.10.2016 um 18:48 schrieb Jeff King:
> > +test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
> > + ln -s does-not-exist .git/broken &&
> > + test_must_fail git rev-parse --verify broken
>
> Hm, lower-case named refs directly in .git are frowned upon, no? If we ever
> decide to forbid them outright, this ref-parse might still fail, but for the
> wrong reason. Should you not better pick an example below ref/?
I suppose so. The test case was adapted from a real-world example, but
it fails equally well with .git/refs/heads/broken. The only restriction
is that the symlink _destination_ cannot look like "refs/heads/...".
Here's a replacement 1/2. The second patch remains unchanged.
-- >8 --
Subject: files_read_raw_ref: avoid infinite loop on broken symlinks
Our ref resolution first runs lstat() on any path we try to
look up, because we want to treat symlinks specially (by
resolving them manually and considering them symrefs). But
if the results of `readlink` do _not_ look like a ref, we
fall through to treating it like a normal file, and just
read the contents of the linked path.
Since fcb7c76 (resolve_ref_unsafe(): close race condition
reading loose refs, 2013-06-19), that "normal file" code
path will stat() the file and if we see ENOENT, will jump
back to the lstat(), thinking we've seen inconsistent
results between the two calls. But for a symbolic ref, this
isn't a race: the lstat() found the symlink, and the stat()
is looking at the path it points to. We end up in an
infinite loop calling lstat() and stat().
We can fix this by avoiding the retry-on-inconsistent jump
when we know that we found a symlink. While we're at it,
let's add a comment explaining why the symlink case gets to
this code in the first place; without that, it is not
obvious that the correct solution isn't to avoid the stat()
code path entirely.
Signed-off-by: Jeff King <peff@peff.net>
---
refs/files-backend.c | 7 ++++++-
t/t1503-rev-parse-verify.sh | 5 +++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0709f60..d826557 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1403,6 +1403,11 @@ static int files_read_raw_ref(struct ref_store *ref_store,
ret = 0;
goto out;
}
+ /*
+ * It doesn't look like a refname; fall through to just
+ * treating it like a non-symlink, and reading whatever it
+ * points to.
+ */
}
/* Is it a directory? */
@@ -1426,7 +1431,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
*/
fd = open(path, O_RDONLY);
if (fd < 0) {
- if (errno == ENOENT)
+ if (errno == ENOENT && !S_ISLNK(st.st_mode))
/* inconsistent with lstat; retry */
goto stat_ref;
else
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index ab27d0d..492edff 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -139,4 +139,9 @@ test_expect_success 'master@{n} for various n' '
test_must_fail git rev-parse --verify master@{$Np1}
'
+test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
+ ln -s does-not-exist .git/refs/heads/broken &&
+ test_must_fail git rev-parse --verify broken
+'
+
test_done
--
2.10.1.506.g904834d
^ permalink raw reply related
* [PATCHv4 2/2] push: change submodule default to check when submodules exist
From: Stefan Beller @ 2016-10-06 19:37 UTC (permalink / raw)
To: gitster; +Cc: git, hvoigt, torvalds, peff, Stefan Beller
In-Reply-To: <20161006193725.31553-1-sbeller@google.com>
When working with submodules, it is easy to forget to push the submodules.
The setting 'check', which checks if any existing submodule is present on
at least one remote of the submodule remotes, is designed to prevent this
mistake.
Flipping the default to check for submodules is safer than the current
default of ignoring submodules while pushing.
However checking for submodules requires additional work[1], which annoys
users that do not use submodules, so we turn on the check for submodules
based on a cheap heuristic, the existence of the .git/modules directory.
That directory doesn't exist when no submodules are used and is only
created and populated when submodules are cloned/added.
When the submodule directory doesn't exist, a user may have changed the
gitlinks via plumbing commands. Currently the default is to not check.
RECURSE_SUBMODULES_DEFAULT is effectively RECURSE_SUBMODULES_OFF currently,
though it may change in the future. When no submodules exist such a check
is pointless as it would fail anyway, so let's just turn it off.
[1] https://public-inbox.org/git/CA+55aFyos78qODyw57V=w13Ux5-8SvBqObJFAq22K+XKPWVbAA@mail.gmail.com/
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/push.c | 15 ++++++++++++++-
t/t5531-deep-submodule-push.sh | 6 +++++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 3bb9d6b..683f270 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -3,6 +3,7 @@
*/
#include "cache.h"
#include "refs.h"
+#include "dir.h"
#include "run-command.h"
#include "builtin.h"
#include "remote.h"
@@ -22,6 +23,7 @@ static int deleterefs;
static const char *receivepack;
static int verbosity;
static int progress = -1;
+static int has_submodules_configured;
static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static enum transport_family family;
@@ -31,6 +33,15 @@ static const char **refspec;
static int refspec_nr;
static int refspec_alloc;
+static void preset_submodule_default(void)
+{
+ if (has_submodules_configured || file_exists(git_path("modules")) ||
+ (!is_bare_repository() && file_exists(".gitmodules")))
+ recurse_submodules = RECURSE_SUBMODULES_CHECK;
+ else
+ recurse_submodules = RECURSE_SUBMODULES_OFF;
+}
+
static void add_refspec(const char *ref)
{
refspec_nr++;
@@ -495,7 +506,8 @@ static int git_push_config(const char *k, const char *v, void *cb)
const char *value;
if (!git_config_get_value("push.recursesubmodules", &value))
recurse_submodules = parse_push_recurse_submodules_arg(k, value);
- }
+ } else if (starts_with(k, "submodule.") && ends_with(k, ".url"))
+ has_submodules_configured = 1;
return git_default_config(k, v, NULL);
}
@@ -552,6 +564,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
};
packet_trace_identity("push");
+ preset_submodule_default();
git_config(git_push_config, &flags);
argc = parse_options(argc, argv, prefix, options, push_usage, 0);
set_push_cert_flags(&flags, push_cert);
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 198ce84..e690749 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -65,7 +65,11 @@ test_expect_success 'push fails if submodule commit not on remote' '
git add gar/bage &&
git commit -m "Third commit for gar/bage" &&
# the push should fail with --recurse-submodules=check
- # on the command line...
+ # on the command line. "check" is the default for repos in
+ # which submodules are detected by existence of config,
+ # .gitmodules file or an internal .git/modules/<submodule-repo>
+ git submodule add -f ../submodule.git gar/bage &&
+ test_must_fail git push ../pub.git master &&
test_must_fail git push --recurse-submodules=check ../pub.git master &&
# ...or if specified in the configuration..
--
2.10.1.353.g1629400
^ permalink raw reply related
* [PATCH 1/2] submodule add: extend force flag to add existing repos
From: Stefan Beller @ 2016-10-06 19:37 UTC (permalink / raw)
To: gitster; +Cc: git, hvoigt, torvalds, peff, Stefan Beller
In-Reply-To: <20161006193725.31553-1-sbeller@google.com>
Currently the force flag in `git submodule add` takes care of possibly
ignored files or when a name collision occurs.
However there is another situation where submodule add comes in handy:
When you already have a gitlink recorded, but no configuration was
done (i.e. no .gitmodules file nor any entry in .git/config) and you
want to generate these config entries. For this situation allow
`git submodule add` to proceed if there is already a submodule at the
given path in the index.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
git-submodule.sh | 10 ++++++++--
t/t7400-submodule-basic.sh | 14 ++++++++++++++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a13..3762616 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -207,8 +207,14 @@ cmd_add()
tstart
s|/*$||
')
- git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
- die "$(eval_gettext "'\$sm_path' already exists in the index")"
+ if test -z "$force"
+ then
+ git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
+ die "$(eval_gettext "'\$sm_path' already exists in the index")"
+ else
+ git ls-files -s "$sm_path" | sane_grep -v "^160000" > /dev/null 2>&1 &&
+ die "$(eval_gettext "'\$sm_path' already exists in the index and is not a submodule")"
+ fi
if test -z "$force" && ! git add --dry-run --ignore-missing "$sm_path" > /dev/null 2>&1
then
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b77cce8..c09ce0d 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -152,6 +152,20 @@ test_expect_success 'submodule add to .gitignored path with --force' '
)
'
+test_expect_success 'submodule add to reconfigure existing submodule with --force' '
+ (
+ cd addtest-ignore &&
+ git submodule add --force bogus-url submod &&
+ git submodule add -b initial "$submodurl" submod-branch &&
+ test "bogus-url" = "$(git config -f .gitmodules submodule.submod.url)" &&
+ test "bogus-url" = "$(git config submodule.submod.url)" &&
+ # Restore the url
+ git submodule add --force "$submodurl" submod
+ test "$submodurl" = "$(git config -f .gitmodules submodule.submod.url)" &&
+ test "$submodurl" = "$(git config submodule.submod.url)"
+ )
+'
+
test_expect_success 'submodule add --branch' '
echo "refs/heads/initial" >expect-head &&
cat <<-\EOF >expect-heads &&
--
2.10.1.353.g1629400
^ permalink raw reply related
* [PATCH 0/2] submodule pushes be extra careful
From: Stefan Beller @ 2016-10-06 19:37 UTC (permalink / raw)
To: gitster; +Cc: git, hvoigt, torvalds, peff, Stefan Beller
This is a reroll of
http://public-inbox.org/git/20161004210359.15266-1-sbeller@google.com/
([PATCHv3 1/2] push: change submodule default to check when submodules exist)
but with a test.
As we only have a heuristic, the test failed initially as these tests don't
have any configuration at all nor do they have the submodule repos in the
superprojects git dir. So I was looking for a cheap way to add a config.
I could have written the config myself via git config, I think it is more
idiomatic to use a submodule command for that. However just getting the
configuration added is not possible with a submodule command, so I made one
in the first patch.
Thanks,
Stefan
Stefan Beller (2):
submodule add: extend force flag to add existing repos
push: change submodule default to check when submodules exist
builtin/push.c | 15 ++++++++++++++-
git-submodule.sh | 10 ++++++++--
t/t5531-deep-submodule-push.sh | 6 +++++-
t/t7400-submodule-basic.sh | 14 ++++++++++++++
4 files changed, 41 insertions(+), 4 deletions(-)
--
2.10.1.353.g1629400
^ permalink raw reply
* Re: [PATCH 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks
From: Johannes Sixt @ 2016-10-06 19:31 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty
In-Reply-To: <20161006164825.otms5ovz2vzanimw@sigill.intra.peff.net>
Am 06.10.2016 um 18:48 schrieb Jeff King:
> +test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
> + ln -s does-not-exist .git/broken &&
> + test_must_fail git rev-parse --verify broken
Hm, lower-case named refs directly in .git are frowned upon, no? If we
ever decide to forbid them outright, this ref-parse might still fail,
but for the wrong reason. Should you not better pick an example below ref/?
-- Hannes
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Jeff King @ 2016-10-06 19:28 UTC (permalink / raw)
To: Rich Felker
Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin, Git,
musl
In-Reply-To: <20161006192500.GS19318@brightrain.aerifal.cx>
On Thu, Oct 06, 2016 at 03:25:00PM -0400, Rich Felker wrote:
> > No, I think that is the exact purpose of configure.ac and autoconf.
> >
> > It would be neat if we could auto-fallback during the build. Rich
> > suggested always compiling compat/regex.c, and just having it be a noop
> > at the preprocessor level. I'm not sure if that would work, though,
> > because we'd have to include the system "regex.h" to know if we have
> > REG_STARTEND, at which point it is potentially too late to compile our
> > own regex routines (we're potentially going to conflict with the system
> > declarations).
>
> If you have autoconf testing for REG_STARTEND at configure time then
> compat/regex.c can #include "config.h" and test for HAVE_REG_STARTEND
> rather than for REG_STARTEND, or something like that.
Right, that part is easy; we do not even have to touch compat/regex.c,
because we already have such a knob in the Makefile (NO_REGEX), and
autoconf just needs to tweak that knob.
My question was whether we could do it without running a separate
compile (via autoconf or via the Makefile), and I think the answer is
"no".
-Peff
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Rich Felker @ 2016-10-06 19:25 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin, Git,
musl
In-Reply-To: <20161006192339.3yddgxxk7jn7zfqx@sigill.intra.peff.net>
On Thu, Oct 06, 2016 at 03:23:40PM -0400, Jeff King wrote:
> On Thu, Oct 06, 2016 at 09:18:29PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> > On Tue, Oct 4, 2016 at 6:08 PM, Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > > As to making NO_REGEX conditional on REG_STARTEND: you are talking about
> > > apples and oranges here. NO_REGEX is a Makefile flag, while REG_STARTEND
> > > is a C preprocessor macro.
> > >
> > > Unless you can convince the rest of the Git developers (you would not
> > > convince me) to simulate autoconf by compiling an executable every time
> > > `make` is run, to determine whether REG_STARTEND is defined, this is a
> > > no-go.
> >
> > But just to clarify, does anyone have any objection to making our
> > configure.ac compile a C program to check for this sort of thing?
> > Because that seems like the easiest solution to this class of problem.
>
> No, I think that is the exact purpose of configure.ac and autoconf.
>
> It would be neat if we could auto-fallback during the build. Rich
> suggested always compiling compat/regex.c, and just having it be a noop
> at the preprocessor level. I'm not sure if that would work, though,
> because we'd have to include the system "regex.h" to know if we have
> REG_STARTEND, at which point it is potentially too late to compile our
> own regex routines (we're potentially going to conflict with the system
> declarations).
If you have autoconf testing for REG_STARTEND at configure time then
compat/regex.c can #include "config.h" and test for HAVE_REG_STARTEND
rather than for REG_STARTEND, or something like that.
Rich
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Junio C Hamano @ 2016-10-06 19:24 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Christian Couder
In-Reply-To: <b0e43079-498a-469b-c4b6-f4fc2b8e2aa4@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> How important is this feature? It doesn't seem too difficult to add,
> although it does break compatibility (in particular, "--signoff" must
> now be documented as "after the last trailer" instead of "at the end
> of the commit message").
The sign-off has always been meant to be appended at the end of the
trailer block, so there is no "compatibility" issue. It's just how
you phrase the explanation of the behaviour, I would think.
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Jeff King @ 2016-10-06 19:23 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Johannes Schindelin, Rich Felker, Git, musl
In-Reply-To: <CACBZZX4XPqZauD_M_ieOwVauT1fi3MQb4+6taELQaRG9M-Kz_w@mail.gmail.com>
On Thu, Oct 06, 2016 at 09:18:29PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Oct 4, 2016 at 6:08 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > As to making NO_REGEX conditional on REG_STARTEND: you are talking about
> > apples and oranges here. NO_REGEX is a Makefile flag, while REG_STARTEND
> > is a C preprocessor macro.
> >
> > Unless you can convince the rest of the Git developers (you would not
> > convince me) to simulate autoconf by compiling an executable every time
> > `make` is run, to determine whether REG_STARTEND is defined, this is a
> > no-go.
>
> But just to clarify, does anyone have any objection to making our
> configure.ac compile a C program to check for this sort of thing?
> Because that seems like the easiest solution to this class of problem.
No, I think that is the exact purpose of configure.ac and autoconf.
It would be neat if we could auto-fallback during the build. Rich
suggested always compiling compat/regex.c, and just having it be a noop
at the preprocessor level. I'm not sure if that would work, though,
because we'd have to include the system "regex.h" to know if we have
REG_STARTEND, at which point it is potentially too late to compile our
own regex routines (we're potentially going to conflict with the system
declarations).
-Peff
^ permalink raw reply
* Re: [PATCH v2 05/25] sequencer: allow the sequencer to take custody of malloc()ed data
From: Junio C Hamano @ 2016-10-06 19:23 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610051338530.35196@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> If you prefer to accept such sloppy work, I will change it of course,
> feeling dirty that it has my name on it.
I do want neither leaks nor sloppyness, and I thought that was
understood by everybody, hence I thought the last round made it
clear that _entrust() must not exist.
Let's try again.
We manage lifetime of a field in a structure in one of three ways in
our codebase [*1*].
* A field can always point at a borrowed piece of memory that the
destruction of the containing structure or re-assignment to the
field do not have to free the current value of the field. This
is a minority, simply because it is rare for a field to be always
able to borrow from others. The destruction of the containing
structure or re-assignment to the field needs to do nothing
special.
* A field can own the piece of memory that it points at. The
destruction of the containing structure or re-assignment to the
field needs to free the current value of the field [*2*]. A
field that can be assigned a value from the configuration (which
is typically xstrdup()'ed) is an example of such a field, and if
a command line option also can assign to the field, we'd need to
xstrdup() it, even though we know we could borrow from argv[],
because cleaning-up needs to be able to free(3) it.
* A field can sometimes own and sometimes borrow the memory, and it
is accompanied by another field to tell which case it is, so that
cleaning-up can tell when it needs to be free(3)d. This is a
minority case, and we generally avoid it especially in modern
code for small allocation, as it makes the lifetime rule more
complex than it is worth.
The _entrust() thing may have been an excellent fourth option if it
were cost-free. xstrdup() that the second approach necessitates for
literal strings (like part of argv[]) would become unnecessary and
we can treat as if all the fields in a structure were all borrowing
the memory from elsewhere (in fact, _entrust() creates the missing
owners of pieces of memory that need to be freed later); essentially
it turns a "mixed ownership" case into the first approach.
But it is not cost-free. The mechanism needs to allocate memory to
become the missing owner and keep track of which pieces of memory
need to be freed later, and the resulting code does not become any
easier to follow. The programmer still needs to know which ones to
_entrust() just like the programmer in the model #2 needs to make
sure xstrdup() is done for appropriate pieces of memory--the only
difference is that an _entrust() programmer needs to mark the pieces
of memory to be freed, while a #2 programmer needs to xstrdup() the
pieces of memory that are being "borrowed".
So let's not add the fourth way to our codebase. "mixed ownership"
case should be turned into "field owns the memory", i.e. approach #2.
[Footnotes]
*1* It is normal for different fields in the same structure follow
different lifetime rules.
*2* Unless leaks are allowed, that is. I think we have instances
where "git cmd --opt=A --opt=B" allocates and stores a new piece
of memory that is computed based on A and store it to a field,
and then overwrites the field with another allocation of a value
based on B without freeing old value, saying "the caller can
avoid passing the same thing twice, and such a leak is miniscule
anyway". That is not nice, and we've been reducing them over
time.
^ permalink raw reply
* Re: Regression: git no longer works with musl libc's regex impl
From: Ævar Arnfjörð Bjarmason @ 2016-10-06 19:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Rich Felker, Jeff King, Git, musl
In-Reply-To: <alpine.DEB.2.20.1610041802310.35196@virtualbox>
On Tue, Oct 4, 2016 at 6:08 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> As to making NO_REGEX conditional on REG_STARTEND: you are talking about
> apples and oranges here. NO_REGEX is a Makefile flag, while REG_STARTEND
> is a C preprocessor macro.
>
> Unless you can convince the rest of the Git developers (you would not
> convince me) to simulate autoconf by compiling an executable every time
> `make` is run, to determine whether REG_STARTEND is defined, this is a
> no-go.
But just to clarify, does anyone have any objection to making our
configure.ac compile a C program to check for this sort of thing?
Because that seems like the easiest solution to this class of problem.
^ permalink raw reply
* Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
From: Junio C Hamano @ 2016-10-06 19:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8D7c8Z_ugasn_scf391+C6GxJp1CYwHY4ndvVtLiJzxnQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> We don't use it internally _yet_. I need to go through all the
>>> external diff code and see --shift-ita should be there. The end goal
>>> is still changing the default behavior and getting rid of --shift-ita,
>>
>> I do not agree with that endgame, and quite honestly I do not want
>> to waste time reviewing such a series.
I definitely shouldn't have said that, especially "waste". Many
issues around i-t-a and diff make my head hurt when I think about
them [*1*], but not wanting to spend time that gets my
head hurt and not wanting to waste time are totally different
things. My apologies.
I missed something curious in your statement above, i.e. "external
diff". I thought we have pretty much got rid of all the invocation
of "git diff" via the run_command() interface and we do not need the
command line option (we only need the options->shift_ita so that
callers like "git status" can seletively ask for it when making
internal calls), and that is why I didn't want to see it.
> I do not believe current "diff" behavior wrt. i-t-a entries is right
> either. There's no point in pursuing this series then. Feel free to
> revert 3f6d56d (commit: ignore intent-to-add entries instead of
> refusing - 2012-02-07) and bring back the old i-t-a behavior. All the
> problems would go away.
It is way too late to revert 3f6d56d. Even though I think it was
overall a mistake to treat i-t-a as "not exist" while committing,
people are now used to the behaviour with that change, and we need
to make our progress within the constraint of the real world.
[Footnote]
Here is one of the things around i-t-a and diff. If you make "git
diff" (between the index and the working tree) report "new" file, it
would imply that "git apply" run without "--index" should create an
ita entry in the index for symmetry, wouldn't it? That by itself
can be seen as an improvement (we no longer would have to say that
"git apply patchfile && git commit -a" that is run in a clean state
will forget new files the patchfile creates), but it also means we
now need a repository in order to run "git apply" (without "--index"),
which is a problem, as "git apply" is often used as a better "patch".
"git apply --cached" may also become "interesting". A patch that
would apply cleanly to HEAD should apply cleanly if you did this:
$ git read-tree HEAD
$ git apply --cached <patch
no matter what the working tree state is. Should a patch that
creates a "new" file add contents to the index, or just an i-t-a
entry? I could argue it both ways, but either is quite satisfactory
and makes my head hurt.
^ permalink raw reply
* Re: Systems with old regex system headers/libraries don't pick up git's compat/regex header file
From: Jeff King @ 2016-10-06 19:11 UTC (permalink / raw)
To: Richard Lloyd; +Cc: git
In-Reply-To: <9f43a2f1-5d7e-3a2e-5a83-40e92ab0d7b5@connectinternetsolutions.com>
On Thu, Oct 06, 2016 at 10:15:49AM +0100, Richard Lloyd wrote:
> Unfortunately, on systems with an older regex shipped as
> standard (e.g. HP-UX 11), the include path picks up
> /usr/include/regex.h first, which doesn't define REG_STARTEND
> and the git-compat-util.h check fails.
>
> The fix I applied on HP-UX 11 was to add -Icompat/regex
> to the CFLAGS ahead of other -I directives. Another possible
> change needed might be to line 69 of compat/regex/regex.c:
Junio mentioned the NO_REGEX knob in the Makefile. If that works for
you, the next step is probably to add a line to the HP-UX section of
config.mak.uname, so that it just works out of the box.
-Peff
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jeff King @ 2016-10-06 19:07 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161006190014.owmqr2eqyk5h5cau@sigill.intra.peff.net>
On Thu, Oct 06, 2016 at 03:00:14PM -0400, Jeff King wrote:
> PS I think your "!!" syntax conflicts with something like:
>
> git -c alias.has-changes='!! git diff --quiet' has-changes
>
> I don't know if that is worth worrying about or not. I also notice
> that using "!!git diff" with no space seems broken, as it seems to
> skip using the shell. I wonder if that is a bug in run-command.
Nevermind this last bit. It is the shell that is complaining
(rightfully) about "!git"; the error message is just confusing because
it mentions $0, which contains the whole script:
$ git -c alias.has-changes='!!git diff --quiet' has-changes
!git diff --quiet: 1: !git diff --quiet: !git: not found
The "!! git diff" syntax sill has the conflict I mentioned (though note
I screwed up the quotes in what I wrote above).
-Peff
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Jeff King @ 2016-10-06 19:00 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161006114124.4966-1-pclouds@gmail.com>
On Thu, Oct 06, 2016 at 06:41:24PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Throwing something at the mailing list to see if anybody is
> interested.
>
> Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make
> handling path arguments hard because they are relative to the original
> cwd. We set GIT_PREFIX to work around it, but I still think it's more
> natural to keep cwd where it is.
>
> We have a way to do that now after 441981b (git: simplify environment
> save/restore logic - 2016-01-26). It's just a matter of choosing the
> right syntax. I'm going with '!!'. I'm not very happy with it. But I
> do like this type of alias.
Hmm. I wonder if any commands will be fooled by not being moved to
GIT_WORK_TREE. I know that there is a bug already in this case:
export HOME=$PWD
for i in one two; do
git init $i &&
(cd $i &&
echo "* diff=$i" >.gitattributes &&
git add . &&
git commit -m $i) &&
git config --global diff.$i.textconv "sed s/^/$i:/"
done
cd two &&
git --git-dir=$(pwd)/../one/.git --work-tree=$(pwd)/../one show
This shows the contents of repo one using the .gitattributes from repo
two. I am not sure if the bug is that when GIT_WORK_TREE is set,
git-show doesn't position itself at the toplevel of that tree, or if the
attributes machinery should be more careful about looking up paths in
the work-tree versus the current directory.
This bug is tangential to your patch (it has nothing to do with aliases,
and should be fixed regardless). But I wonder if your "!!" aliases will
expose this bug more frequently.
-Peff
PS I think your "!!" syntax conflicts with something like:
git -c alias.has-changes="!! git diff --quiet' has-changes
I don't know if that is worth worrying about or not. I also notice
that using "!!git diff" with no space seems broken, as it seems to
skip using the shell. I wonder if that is a bug in run-command.
^ permalink raw reply
* Re: Format for %d includes unwanted preceding space
From: Junio C Hamano @ 2016-10-06 18:49 UTC (permalink / raw)
To: Ravi (Tom) Hale; +Cc: git
In-Reply-To: <a3eed8fb-505c-e9c1-6fb6-944d9df2f15d@hale.ee>
"Ravi (Tom) Hale" <ravi@hale.ee> writes:
> The log --format="%d" includes preceding space:
>
> $ git log -n1 --format="XX%dXX" HEAD
> XX (HEAD -> master)XX
>
> I know of no other %<token> that is output in this way.
That is primarily because %d predates %[-+ ]<placeholder> mechanism,
I think. It is too late to redefine what "%d" means and force all
users to update their script to use "% d" instead, unfortunately.
^ permalink raw reply
* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Junio C Hamano @ 2016-10-06 18:42 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161006114124.4966-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Throwing something at the mailing list to see if anybody is
> interested.
>
> Current '!' aliases move cwd to $GIT_WORK_TREE first, which could make
> handling path arguments hard because they are relative to the original
> cwd. We set GIT_PREFIX to work around it, but I still think it's more
> natural to keep cwd where it is.
>
> We have a way to do that now after 441981b (git: simplify environment
> save/restore logic - 2016-01-26). It's just a matter of choosing the
> right syntax. I'm going with '!!'. I'm not very happy with it. But I
> do like this type of alias.
I do not know why you are not happy with the syntax, but I
personally think it brilliant, both the idea and the preliminary
clean-up that made this possible with a simple patch like this.
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> git.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/git.c b/git.c
> index 296857a..4c1dcf4 100644
> --- a/git.c
> +++ b/git.c
> @@ -252,6 +252,10 @@ static int handle_alias(int *argcp, const char ***argv)
>
> alias_string++;
> commit_pager_choice();
> + if (*alias_string == '!') {
> + keep_cwd = 0;
> + alias_string++;
> + }
> restore_env(keep_cwd);
>
> child.use_shell = 1;
^ permalink raw reply
* Re: [PATCH v2 20/25] sequencer: left-trim lines read from the script
From: Junio C Hamano @ 2016-10-06 18:41 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jakub Narębski
In-Reply-To: <5ed8aaea-9fbe-11b0-49b7-1b42567d4918@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Let me take the opportunity to say
>
> Thank You Very Much!
>
> for the new implementation of rebase -i. I'm using your branch
> rebase-i-extra, and it is such a pleasure to work with on Windows
> compared to the old fully scripted version.
Thanks for testing.
Having more guinea pigs ^W ^W testers before the series is reviewed
here would be a nice boost and would hopefully encourage more
reviewers to help the series into a good shape to be upstreamed ;-)
> ---- 8< ----
> [PATCH] sequencer: strip CR from the end of exec insns
>
> It is not unheard of that editors on Windows write CRLF even if the file
> originally had only LF. This is particularly awkward for exec lines of a
> rebase -i todo sheet. Take for example the insn "exec echo": The shell
> script parser (either of the sequencer or of the shell that is invoked,
> I do not know) splits at the LF and leaves the CR attached to "echo",
> which leads to the unknown command "echo\r".
Interesting find. So it's not just ltrim is being lenient only to
end-user typo, but not doing rtrim can actively hurt ;-)
> Work it around by stripping CR before the command specified with exec is
> passed to the shell.
Makes sense.
^ permalink raw reply
* Re: Systems with old regex system headers/libraries don't pick up git's compat/regex header file
From: Junio C Hamano @ 2016-10-06 18:25 UTC (permalink / raw)
To: Richard Lloyd; +Cc: git
In-Reply-To: <9f43a2f1-5d7e-3a2e-5a83-40e92ab0d7b5@connectinternetsolutions.com>
Richard Lloyd <richard.lloyd@connectinternetsolutions.com> writes:
> git ships with a compat/regex tree containing a recent regex
> release, presumably with the intention of allowing systems with
> either no regex or an old regex installed to use the newer compat
> version.
Quick question. Does NO_REGEX help? cf. Makefile
# Define NO_REGEX if your C library lacks regex support with REG_STARTEND
# feature.
^ permalink raw reply
* Re: [PATCH] push: Re-include "push.default=tracking" in the documentation
From: Junio C Hamano @ 2016-10-06 18:24 UTC (permalink / raw)
To: Matthieu Moy
Cc: Ævar Arnfjörð Bjarmason, Git, Johan Herland,
Ramkumar Ramachandra
In-Reply-To: <vpqlgy1h28g.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Junio, looks like from the 2013 discussion that you preferred just
>> having that mention in parenthesis instead of its own item, how about
>> just re-applying your fa23348 (with conflicts resolved)?
>
> (fa23348 did this:
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1749,7 +1749,8 @@ push.default::
> +
> This is currently the default, but Git 2.0 will change the default
> to `simple`.
> -* `upstream` - push the current branch to its upstream branch.
> +* `upstream` - push the current branch to its upstream branch
> + (`tracking` is a deprecated synonym for this).
> )
>
> I agree that doing the same thing is the best option.
Sorry, I wasn't paying attention to this thread.
It seems that 87a70e4ce8 ("config doc: rewrite push.default
section", 2013-06-19) removed that mention by accident? The log
message of the commit does not say it actively wanted to remove
mention of `tracking` and/or why it wanted to do so, so I agree that
resurrecting that parenthetical mention is the easiest course of
action at this point.
However.
With today's description of push.default choices, each of which is a
full fledged paragraph, I no longer have the objection I had in
https://public-inbox.org/git/7vip6dgmx2.fsf@alter.siamese.dyndns.org/
against having `tracking` as a separate bullet item. If we add
* `tracking` - a deprecated synonym for `upstream`; do not use this.
to today's list, it would stand out as something different from
others and it will not cause the confusion I feared in the
discussion we had in early 2013. As Jonathan Nieder argued in the
thread back then, having it as one of the bullet point would help
people locate it without using "search" \C-s or / feature.
^ permalink raw reply
* Re: [PATCH 6/6] Documentation/git-merge.txt: get rid of irrelevant references to git-pull
From: Junio C Hamano @ 2016-10-06 18:06 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87h98pps7a.fsf@javad.com>
Sergey Organov <sorganov@gmail.com> writes:
> Ah, I now see. I tried to keep the text intact as much as possible, and
> only split it into description and a note. Well, how about this then:
Much better than your earlier patch, but I am not sure if the
updated one is that much better compared to the original.
The pre- and post- state of this "how about this" patch essentially
say the same thing, and I suspect that the primary reason why you
think the post- state is easier to read is because you wrote it,
while the reason why I do not see much difference is because I
didn't write the updated one ;-).
I do find "In this case, ... store the combined history" in the
original a bit awkward to read, but most of that awkardness is
inherited by the updated text. It may benefit from hinting why a
new commit is not needed a bit stronger. Here is my attempt:
When the commit we are merging is a descendant of the current
HEAD, the history leading to the named commit can be, and by
default is, taken as the combined history of the two. Our
history is "fast forwarded" to their history by updating `HEAD`
along with the index to point at the named commit.
This often happens when you are following along somebody else's
work via "git pull" without doing your own development.
I think the awkwardness I felt in the original and your version is
gone from the above attempt, but I doubt that it is better over
either of them in any other way.
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index b758d55..479400f 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -135,15 +135,17 @@ will exit early with the message "Already up-to-date."
> FAST-FORWARD MERGE
> ------------------
>
> -Often the current branch head is an ancestor of the named commit.
> -This is the most common case especially when invoked from 'git
> -pull': you are tracking an upstream repository, you have committed
> -no local changes, and now you want to update to a newer upstream
> -revision. In this case, a new commit is not needed to store the
> +When current branch head is an ancestor of the named commit,
> +a new commit is not needed to store the
> combined history; instead, the `HEAD` (along with the index) is
> updated to point at the named commit, without creating an extra
> merge commit.
>
> +This is very common case when 'git merge' is invoked from 'git
> +pull': you are tracking an upstream repository, you have committed
> +no local changes, and now you want to update to a newer upstream
> +revision.
> +
> This behavior can be suppressed with the `--no-ff` option.
>
> TRUE MERGE
>
> -- Sergey
^ 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