* Re: [PATCH 0/1] add author and committer configuration settings
From: Jonathan Nieder @ 2018-12-19 21:46 UTC (permalink / raw)
To: William Hubbs; +Cc: git, chutzpah
In-Reply-To: <20181219183939.16358-1-williamh@gentoo.org>
Hi,
William Hubbs wrote:
> this is my first patch for git, so please be gentle. ;-)
Thanks for contributing!
> I am in a situation where I need to use different authorship information
> for some repositories I commit to.
>
> Git already supports setting different authorship and committer
> information with environment variables, but this setting is global so if
> you want to change it per repository you need to use a separate tool.
>
> This patch adds support to git config for author.email, author.name,
> committer.email and committer.name settings so this information
> can be set per repository. It applies to current master.
The above information (except for "It applies to current master") is
very useful to have when looking back at the change in history. When
sending the next version of this patch in response to others'
comments, can you replace the commit message with something like it?
In other words, it is very useful for the commit message to include
this kind of information about the "why" behind a change, beyond the
"what" that the patch itself already provides.
> Thanks much for your reviews, and I would like to publically thank dscho
> from #git-devel for assisting me in preparing this patch.
>
> Also, since I use a screen reader, it would be very helpful if you put
> your comments in your replies as close to the affected code as possible,
> preferably directly below it.
Fortunately, this is already common practice here, but the reminder is
very welcome.
By the way, if you have other feedback about Git accessibility through
a screen reader (both the project and the tool), I would be very
interested to hear. Presumably in a new thread. :)
Thanks and hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH 2/3] setup: do not use invalid `repository_format`
From: Martin Ågren @ 2018-12-19 21:43 UTC (permalink / raw)
To: brian m. carlson, Git Mailing List; +Cc: Jeff King
In-Reply-To: <20181219001801.GA520988@genre.crustytoothpaste.net>
On Wed, 19 Dec 2018 at 01:18, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> I think this change is fine, because we initialize the value in
> the_repository elsewhere, and if there's no repository, this should
> never have a value other than the default anyway.
Thanks, it feels good that this patch matches how you think about the
`hash_algo` field.
> I looked at the other patches in the series and thought they looked sane
> as well.
Thanks for a review, I appreciate it.
Martin
^ permalink raw reply
* Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`
From: Martin Ågren @ 2018-12-19 21:42 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, brian m . carlson
In-Reply-To: <20181219152735.GA14802@sigill.intra.peff.net>
On Wed, 19 Dec 2018 at 16:27, Jeff King <peff@peff.net> wrote:
>
> On Tue, Dec 18, 2018 at 08:25:26AM +0100, Martin Ågren wrote:
>
> > No-one looks at the return value, so we might as well drop it. It's
> > still available as `format->version`.
>
> Hmm. If we have to pick one, I'd say that just returning a sane exit
> value would be the more conventional thing to do. But looking at the
> callers, many of them want to just pass the struct on to the verify
> function.
>
> That said, there is a long-standing curiosity here that we may want to
> consider: read_repository_format() does not distinguish between these
> three cases:
[snip several valuable insights]
> I dunno. This is one of those dark corners of the code where we appear
> to do the wrong thing, but nobody seems to have noticed or cared much,
> and changing it runs the risk of breaking some obscure cases. I'm not
> sure if we should bite the bullet and try to address that, or just back
> away slowly and pretend we never looked at it. ;)
That was my reaction when I first dug into this. :-/ It's only now that
I've been brave enough to even try to dig through the first layer.
> FWIW, the patch itself seems fine, and obviously doesn't make anything
> worse on its own. The question is just whether we want to do more
> cleanup here.
Well, if we do want to make more cleanups around here, one of the more
obvious ideas is to actually use the return value. If such a cleanup is
going to start with a (moral) revert of this patch, then we're probably
better off just dropping this patch.
Thanks for your thoughtful response
Martin
^ permalink raw reply
* [PATCH 1/1] commit-graph: writing missing parents is a BUG
From: Derrick Stolee via GitGitGadget @ 2018-12-19 20:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee
In-Reply-To: <pull.102.git.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
When writing a commit-graph, we write GRAPH_MISSING_PARENT if the
parent's object id does not appear in the list of commits to be
written into the commit-graph. This was done as the initial design
allowed commits to have missing parents, but the final version
requires the commit-graph to be closed under reachability. Thus,
this GRAPH_MISSING_PARENT value should never be written.
However, there are reasons why it could be written! These range
from a bug in the reachable-closure code to a memory error causing
the binary search into the list of object ids to fail. In either
case, we should fail fast and avoid writing the commit-graph file
with bad data.
Remove the GRAPH_MISSING_PARENT constant in favor of the constant
GRAPH_EDGE_LAST_MASK, which has the same value.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
commit-graph.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 40c855f185..c14ada6918 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -34,7 +34,6 @@
#define GRAPH_OID_LEN GRAPH_OID_LEN_SHA1
#define GRAPH_OCTOPUS_EDGES_NEEDED 0x80000000
-#define GRAPH_PARENT_MISSING 0x7fffffff
#define GRAPH_EDGE_LAST_MASK 0x7fffffff
#define GRAPH_PARENT_NONE 0x70000000
@@ -496,7 +495,9 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
commit_to_sha1);
if (edge_value < 0)
- edge_value = GRAPH_PARENT_MISSING;
+ BUG("missing parent %s for commit %s",
+ oid_to_hex(&parent->item->object.oid),
+ oid_to_hex(&(*list)->object.oid));
}
hashwrite_be32(f, edge_value);
@@ -514,7 +515,9 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
nr_commits,
commit_to_sha1);
if (edge_value < 0)
- edge_value = GRAPH_PARENT_MISSING;
+ BUG("missing parent %s for commit %s",
+ oid_to_hex(&parent->item->object.oid),
+ oid_to_hex(&(*list)->object.oid));
}
hashwrite_be32(f, edge_value);
@@ -567,7 +570,9 @@ static void write_graph_chunk_large_edges(struct hashfile *f,
commit_to_sha1);
if (edge_value < 0)
- edge_value = GRAPH_PARENT_MISSING;
+ BUG("missing parent %s for commit %s",
+ oid_to_hex(&parent->item->object.oid),
+ oid_to_hex(&(*list)->object.oid));
else if (!parent->next)
edge_value |= GRAPH_LAST_EDGE;
@@ -864,7 +869,7 @@ void write_commit_graph(const char *obj_dir,
count_distinct++;
}
- if (count_distinct >= GRAPH_PARENT_MISSING)
+ if (count_distinct >= GRAPH_EDGE_LAST_MASK)
die(_("the commit graph format cannot write %d commits"), count_distinct);
commits.nr = 0;
@@ -891,7 +896,7 @@ void write_commit_graph(const char *obj_dir,
}
num_chunks = num_extra_edges ? 4 : 3;
- if (commits.nr >= GRAPH_PARENT_MISSING)
+ if (commits.nr >= GRAPH_EDGE_LAST_MASK)
die(_("too many commits to write graph"));
compute_generation_numbers(&commits, report_progress);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/1] commit-graph: writing missing parents is a BUG
From: Derrick Stolee via GitGitGadget @ 2018-12-19 20:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
A user complained that they had the following message in a git command:
fatal: invalid parent position 2147483647
In hex, this value is 0x7fffffff, corresponding to the GRAPH_MISSING_PARENT
constant. This constant was intended as a way to have the commit-graph store
commits with parents that are not also in the commit-graph. During
development, however, we chose to require the commit-graph to be closed
under reachability. Thus, this value should never be written, and we don't
fall back to parsing usual commits when we see the constant.
This actually happened, and the root cause is unknown. This either means the
reachable closure logic is broken somewhere, or something caused the binary
search to find the parent in our list of commits. This second problem is
more likely, as we have seen RAM issues cause corrupted memory before. I'm
still investigating the root cause, but for now we can hit a BUG() statement
instead of writing bad data.
Thanks, -Stolee
Derrick Stolee (1):
commit-graph: writing missing parents is a BUG
commit-graph.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
base-commit: 0d0ac3826a3bbb9247e39e12623bbcfdd722f24c
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-102%2Fderrickstolee%2Fcommit-graph-bug-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-102/derrickstolee/commit-graph-bug-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/102
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 1/1] Add author and committer configuration settings
From: John Passaro @ 2018-12-19 20:05 UTC (permalink / raw)
To: William Hubbs; +Cc: git, chutzpah
In-Reply-To: <20181219183939.16358-2-williamh@gentoo.org>
On Wed, Dec 19, 2018 at 2:19 PM William Hubbs <williamh@gentoo.org> wrote:
>
> The author.email, author.name, committer.email and committer.name
> settings are analogous to the GIT_AUTHOR_* and GIT_COMMITTER_*
> environment variables, but for the git config system. This allows them
> to be set separately for each repository.
>
Great! I didn't realize this wasn't already supported...
However your patch does seem to be missing tests.
t/t7517-per-repo-email.sh would appear to be a logical place to add
them.
> Signed-off-by: William Hubbs <williamh@gentoo.org>
> ---
> Documentation/config/user.txt | 20 +++++++++++
> builtin/commit.c | 2 +-
> cache.h | 5 ++-
> config.c | 6 ++++
> ident.c | 68 ++++++++++++++++++++++++++++++++---
> log-tree.c | 3 +-
> sequencer.c | 3 +-
> 7 files changed, 97 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> index b5b2ba1199..6ba7002252 100644
> --- a/Documentation/config/user.txt
> +++ b/Documentation/config/user.txt
> @@ -1,3 +1,23 @@
> +author.email::
> +Your email address to be recorded on the author line of any newly
> +created commits.
> +If this is not set, we use user.email.
> +
> +author.name::
> +Your full name to be recorded on the author line of any newly
> +created commits.
> +If this is not set, we use user.name.
> +
> +committer.email::
> +Your email address to be recorded on the committer line of any newly
> +created commits.
> +If this is not set, we use user.email.
> +
> +committer.name::
> +Your full name to be recorded on the committer line of any newly
> +created commits.
> +If this is not set, we use user.name.
> +
> user.email::
> Your email address to be recorded in any newly created commits.
> Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and
I think it would be wise to mention the new config items under
user.email and user.name as well.
^ permalink raw reply
* Re: error: Use of uninitialized value $hash in chomp
From: Ævar Arnfjörð Bjarmason @ 2018-12-19 19:00 UTC (permalink / raw)
To: Andrew Shearer; +Cc: git@vger.kernel.org, Eric Wong
In-Reply-To: <ME1PR01MB11218735004432E3E26C66FFE1BE0@ME1PR01MB1121.ausprd01.prod.outlook.com>
On Wed, Dec 19 2018, Andrew Shearer wrote:
> Hello
>
> I am using a "git svn clone" command to extract our project history from svn into git.
> About 30m into the process it fails with:
>
> r50739 = 2a1491de1353b1e3cce50d8f9d383407218a44f1 (refs/remotes/git-svn)
> fatal: Cannot open '.git/Git_svn_delta_33316_0_UkxiJV': Permission denied
> Use of uninitialized value $hash in chomp at C:/Program Files/Git/mingw64/share/perl5/Git.pm line 929, <GEN11> line 36311.
> hash-object -w --stdin-paths --no-filters: command returned error: 128
>
> error closing pipe: Bad file descriptor at C:/Program Files/Git/mingw64/libexec/git-core\git-svn line 0.
> error closing pipe: Bad file descriptor at C:/Program Files/Git/mingw64/libexec/git-core\git-svn line 0.
> (in cleanup) at /usr/share/perl5/vendor_perl/Error.pm line 198 during global destruction.
>
> I tried updating to the latest build, 2.20.1.windows, but it still fails.
>
> There is nothing particularly special about svn changeset 50739 that I can see compared to any other.
> Anyone know why this might be failing or how I could resolve it?
That "Permission denied" looks scary. Don't know how git-svn gets into
this, but try with this patch on top:
diff --git a/perl/Git.pm b/perl/Git.pm
index d856930b2e..f5d15895d3 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -926,7 +926,13 @@ sub hash_and_insert_object {
throw Error::Simple("out pipe went bad");
}
- chomp(my $hash = <$in>);
+ my $hash = <$in>;
+ unless (defined $hash) {
+ sub noes { die "blah" }
+ noes();
+ } else {
+ chomp($hash);
+ }
unless (defined($hash)) {
$self->_close_hash_and_insert_object();
throw Error::Simple("in pipe went bad");
Then run:
perl -d $(git --exec-path)/git-svn
Set a breakpoint at that "noes" with:
DB<1> b Git::noes
Continue:
DB<2> c
Then when it stops there get a backtrace with "T":
DB<2> T
@ = DB::DB called from file 'perl/Git.pm' line 931
. = Git::noes() called from file 'perl/Git.pm' line 932
. = Git::hash_and_insert_object(ref(Git), 'Makefile') called from -e line 1
And see if you can get any other relevant info out of the debugger. See
"perldoc perldebug".
^ permalink raw reply
* Re: Referring to commits in commit messages
From: Ævar Arnfjörð Bjarmason @ 2018-12-19 18:52 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Han-Wen Nienhuys, Johannes Schindelin
In-Reply-To: <20181219182216.GA17309@sigill.intra.peff.net>
On Wed, Dec 19 2018, Jeff King wrote:
> On Wed, Dec 19, 2018 at 03:02:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Dec 17 2018, Jonathan Nieder wrote:
>>
>> > v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)
>>
>> Minor nit not just on this patch, but your patches in general: I think
>> you're the only one using this type of template instead of the `%h
>> ("%s", %ad)` format documented in SubmittingPatches.
>>
>> I've had at least a couple of cases where I've git log --grep=<abbr sha>
>> and missed a commit of yours when you referred to another commit.
>>
>> E.g. when composing
>> https://public-inbox.org/git/878t0lfwrj.fsf@evledraar.gmail.com/ I
>> remembered PERLLIB_EXTRA went back & forth between
>> working/breaking/working with your/my/your patch, so:
>>
>> git log --grep=0386dd37b1
>>
>> Just found the chain up to my breaking change, but not your 7a7bfc7adc,
>> which refers to that commit as v1.9-rc0~88^2.
>>
>> Maybe this is really a feature request. I.e. maybe we should have some
>> mode where --grep=<commitish> will be combined with some mode where we
>> try to find various forms of <commitish> in commit messages, then
>> normalize & match them....
>
> That would help when you're using --grep, but not other things that are
> trying to parse the commit message. Two instances I've noticed:
>
> - web interfaces like GitHub won't linkify this type of reference
> (whereas they will for something that looks like a hex object id)
I wonder if we had some canonical plumbing combination of to `git
cat-file -p` and/or a utility like git-interpret-trailers that would
take a commit message and spew out BEGIN/END/SHA-1 positions for
commitish that we found whether sites like GitHub would use it.
They'd still need to do a second pass to for any of their own markup,
e.g. the elsewhere@<commitish> syntax, or referring to PRs/MRs issues
etc....
> - my terminal makes it easy to select hex strings, but doesn't
> understand this git-describe output :)
>
> These tools _could_ be taught a regex like /v(\d+.)(-rc\d+)?([~^]+d)*/.
> But matching hex strings is a lot simpler, and works across many
> projects.
>
> So I agree with you that this git-describe format is less convenient for
> readers, but my preferred solution is to use a different format, rather
> than try to teach every reading tool to be more clever.
>
> As far as I can tell, the main advantage of using "v2.11.0-rc3~3^2~1"
> over its hex id is that it gives a better sense in time of which Git
> version we're talking about. The date in the parentheses does something
> similar for wall-clock time, but it's left to the reader to map that to
> a Git version if they choose.
>
> Personally, I find the wall-clock time to be enough, since usually what
> I want to know is "how ancient is this". And in the rare instance that I
> care about the containing version, it's not a big deal to use "git tag
> --contains". If we really want to convey that information in the text,
> I think it would be reasonable to say something like:
>
> In commit 1234abcd (the subject line, 2016-01-01, v2.11.0), we did
> blah blah blah
>
> with a few simple rules:
>
> - only mention a single version, the oldest one that contains the
> commit[1]. If it's in v2.11.1, we can infer that it's in v2.12.0,
> etc.
>
> - only mention released commits; for the granularity we're talking
> about here, the distinction between v2.11.0 and v2.11.0-rc3 is not
> important
>
> - if it hasn't been in a released version, don't include a version at
> all.
>
> That's probably over-engineering, and I'm perfectly fine with the
> oid/subject/date format most people use. Just trying to give an option
> if people really think the tag name is useful.
>
> -Peff
>
> [1] I usually compute the containing version with:
>
> $ git help has
> 'has' is aliased to '!f() { git tag --contains "$@" | grep ^v | grep -v -- -rc | sort -V | head -1; }; f'
>
> though I suspect it could be done these days with fewer processes
> using "tag --sort".
^ permalink raw reply
* [PATCH 1/1] Add author and committer configuration settings
From: William Hubbs @ 2018-12-19 18:39 UTC (permalink / raw)
To: git; +Cc: williamh, chutzpah
In-Reply-To: <20181219183939.16358-1-williamh@gentoo.org>
The author.email, author.name, committer.email and committer.name
settings are analogous to the GIT_AUTHOR_* and GIT_COMMITTER_*
environment variables, but for the git config system. This allows them
to be set separately for each repository.
Signed-off-by: William Hubbs <williamh@gentoo.org>
---
Documentation/config/user.txt | 20 +++++++++++
builtin/commit.c | 2 +-
cache.h | 5 ++-
config.c | 6 ++++
ident.c | 68 ++++++++++++++++++++++++++++++++---
log-tree.c | 3 +-
sequencer.c | 3 +-
7 files changed, 97 insertions(+), 10 deletions(-)
diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
index b5b2ba1199..6ba7002252 100644
--- a/Documentation/config/user.txt
+++ b/Documentation/config/user.txt
@@ -1,3 +1,23 @@
+author.email::
+Your email address to be recorded on the author line of any newly
+created commits.
+If this is not set, we use user.email.
+
+author.name::
+Your full name to be recorded on the author line of any newly
+created commits.
+If this is not set, we use user.name.
+
+committer.email::
+Your email address to be recorded on the committer line of any newly
+created commits.
+If this is not set, we use user.email.
+
+committer.name::
+Your full name to be recorded on the committer line of any newly
+created commits.
+If this is not set, we use user.name.
+
user.email::
Your email address to be recorded in any newly created commits.
Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and
diff --git a/builtin/commit.c b/builtin/commit.c
index c021b119bb..49a97adeb8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -607,7 +607,7 @@ static void determine_author_info(struct strbuf *author_ident)
set_ident_var(&date, strbuf_detach(&date_buf, NULL));
}
- strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));
+ strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT|IDENT_AUTHOR));
assert_split_ident(&author, author_ident);
export_one("GIT_AUTHOR_NAME", author.name_begin, author.name_end, 0);
export_one("GIT_AUTHOR_EMAIL", author.mail_begin, author.mail_end, 0);
diff --git a/cache.h b/cache.h
index ca36b44ee0..0ee87f22a9 100644
--- a/cache.h
+++ b/cache.h
@@ -1479,10 +1479,13 @@ int date_overflows(timestamp_t date);
#define IDENT_STRICT 1
#define IDENT_NO_DATE 2
#define IDENT_NO_NAME 4
+#define IDENT_AUTHOR 8
+#define IDENT_COMMITTER 16
+
extern const char *git_author_info(int);
extern const char *git_committer_info(int);
extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
-extern const char *fmt_name(const char *name, const char *email);
+extern const char *fmt_committer_name(void);
extern const char *ident_default_name(void);
extern const char *ident_default_email(void);
extern const char *git_editor(void);
diff --git a/config.c b/config.c
index ff521eb27a..4bd5920dea 100644
--- a/config.c
+++ b/config.c
@@ -1484,6 +1484,12 @@ int git_default_config(const char *var, const char *value, void *cb)
return 0;
}
+ if (starts_with(var, "author."))
+ return git_ident_config(var, value, cb);
+
+ if (starts_with(var, "committer."))
+ return git_ident_config(var, value, cb);
+
/* Add other config variables here and to Documentation/config.txt. */
return 0;
}
diff --git a/ident.c b/ident.c
index 33bcf40644..3da96ebbef 100644
--- a/ident.c
+++ b/ident.c
@@ -11,6 +11,10 @@
static struct strbuf git_default_name = STRBUF_INIT;
static struct strbuf git_default_email = STRBUF_INIT;
static struct strbuf git_default_date = STRBUF_INIT;
+static struct strbuf git_author_name = STRBUF_INIT;
+static struct strbuf git_author_email = STRBUF_INIT;
+static struct strbuf git_committer_name = STRBUF_INIT;
+static struct strbuf git_committer_email = STRBUF_INIT;
static int default_email_is_bogus;
static int default_name_is_bogus;
@@ -361,7 +365,15 @@ const char *fmt_ident(const char *name, const char *email,
int strict = (flag & IDENT_STRICT);
int want_date = !(flag & IDENT_NO_DATE);
int want_name = !(flag & IDENT_NO_NAME);
+ int want_author = (flag & IDENT_AUTHOR);
+ int want_committer = (flag & IDENT_COMMITTER);
+ if (!email) {
+ if (want_author && git_author_email.len)
+ email = git_author_email.buf;
+ else if (want_committer && git_committer_email.len)
+ email = git_committer_email.buf;
+ }
if (!email) {
if (strict && ident_use_config_only
&& !(ident_config_given & IDENT_MAIL_GIVEN)) {
@@ -377,6 +389,12 @@ const char *fmt_ident(const char *name, const char *email,
if (want_name) {
int using_default = 0;
+ if (!name) {
+ if (want_author && git_author_name.len)
+ name = git_author_name.buf;
+ else if (want_committer && git_committer_name.len)
+ name = git_committer_name.buf;
+ }
if (!name) {
if (strict && ident_use_config_only
&& !(ident_config_given & IDENT_NAME_GIVEN)) {
@@ -425,9 +443,11 @@ const char *fmt_ident(const char *name, const char *email,
return ident.buf;
}
-const char *fmt_name(const char *name, const char *email)
+const char *fmt_committer_name(void)
{
- return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE);
+ char *name = getenv("GIT_COMMITTER_NAME");
+ char *email = getenv("GIT_COMMITTER_EMAIL");
+ return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE|IDENT_COMMITTER);
}
const char *git_author_info(int flag)
@@ -439,7 +459,7 @@ const char *git_author_info(int flag)
return fmt_ident(getenv("GIT_AUTHOR_NAME"),
getenv("GIT_AUTHOR_EMAIL"),
getenv("GIT_AUTHOR_DATE"),
- flag);
+ flag|IDENT_AUTHOR);
}
const char *git_committer_info(int flag)
@@ -451,7 +471,7 @@ const char *git_committer_info(int flag)
return fmt_ident(getenv("GIT_COMMITTER_NAME"),
getenv("GIT_COMMITTER_EMAIL"),
getenv("GIT_COMMITTER_DATE"),
- flag);
+ flag|IDENT_COMMITTER);
}
static int ident_is_sufficient(int user_ident_explicitly_given)
@@ -480,6 +500,46 @@ int git_ident_config(const char *var, const char *value, void *data)
return 0;
}
+ if (!strcmp(var, "author.name")) {
+ if (!value)
+ return config_error_nonbool(var);
+ strbuf_reset(&git_author_name);
+ strbuf_addstr(&git_author_name, value);
+ author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ ident_config_given |= IDENT_NAME_GIVEN;
+ return 0;
+ }
+
+ if (!strcmp(var, "author.email")) {
+ if (!value)
+ return config_error_nonbool(var);
+ strbuf_reset(&git_author_email);
+ strbuf_addstr(&git_author_email, value);
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ ident_config_given |= IDENT_MAIL_GIVEN;
+ return 0;
+ }
+
+ if (!strcmp(var, "committer.name")) {
+ if (!value)
+ return config_error_nonbool(var);
+ strbuf_reset(&git_committer_name);
+ strbuf_addstr(&git_committer_name, value);
+ committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ ident_config_given |= IDENT_NAME_GIVEN;
+ return 0;
+ }
+
+ if (!strcmp(var, "committer.email")) {
+ if (!value)
+ return config_error_nonbool(var);
+ strbuf_reset(&git_committer_email);
+ strbuf_addstr(&git_committer_email, value);
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ ident_config_given |= IDENT_MAIL_GIVEN;
+ return 0;
+ }
+
if (!strcmp(var, "user.name")) {
if (!value)
return config_error_nonbool(var);
diff --git a/log-tree.c b/log-tree.c
index 10680c139e..6760a2e9c4 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -687,8 +687,7 @@ void show_log(struct rev_info *opt)
*/
if (ctx.need_8bit_cte >= 0 && opt->add_signoff)
ctx.need_8bit_cte =
- has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"),
- getenv("GIT_COMMITTER_EMAIL")));
+ has_non_ascii(fmt_committer_name());
ctx.date_mode = opt->date_mode;
ctx.date_mode_explicit = opt->date_mode_explicit;
ctx.abbrev = opt->diffopt.abbrev;
diff --git a/sequencer.c b/sequencer.c
index e1a4dd15f1..f357defda5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4036,8 +4036,7 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
int has_footer;
strbuf_addstr(&sob, sign_off_header);
- strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
- getenv("GIT_COMMITTER_EMAIL")));
+ strbuf_addstr(&sob, fmt_committer_name());
strbuf_addch(&sob, '\n');
if (!ignore_footer)
--
2.19.2
^ permalink raw reply related
* [PATCH 0/1] add author and committer configuration settings
From: William Hubbs @ 2018-12-19 18:39 UTC (permalink / raw)
To: git; +Cc: williamh, chutzpah
Hi all,
this is my first patch for git, so please be gentle. ;-)
I am in a situation where I need to use different authorship information
for some repositories I commit to.
Git already supports setting different authorship and committer
information with environment variables, but this setting is global so if
you want to change it per repository you need to use a separate tool.
This patch adds support to git config for author.email, author.name,
committer.email and committer.name settings so this information
can be set per repository. It applies to current master.
Thanks much for your reviews, and I would like to publically thank dscho
from #git-devel for assisting me in preparing this patch.
Also, since I use a screen reader, it would be very helpful if you put
your comments in your replies as close to the affected code as possible,
preferably directly below it.
William Hubbs (1):
Add author and committer configuration settings
Documentation/config/user.txt | 20 +++++++++++
builtin/commit.c | 2 +-
cache.h | 5 ++-
config.c | 6 ++++
ident.c | 68 ++++++++++++++++++++++++++++++++---
log-tree.c | 3 +-
sequencer.c | 3 +-
7 files changed, 97 insertions(+), 10 deletions(-)
--
2.19.2
^ permalink raw reply
* Re: Referring to commits in commit messages
From: Jonathan Nieder @ 2018-12-19 18:39 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Han-Wen Nienhuys,
Johannes Schindelin
In-Reply-To: <20181219182216.GA17309@sigill.intra.peff.net>
Hi,
Jeff King wrote:
> - web interfaces like GitHub won't linkify this type of reference
> (whereas they will for something that looks like a hex object id)
>
> - my terminal makes it easy to select hex strings, but doesn't
> understand this git-describe output :)
>
> These tools _could_ be taught a regex like /v(\d+.)(-rc\d+)?([~^]+d)*/.
> But matching hex strings is a lot simpler, and works across many
> projects.
Is there some rule about how long the hex string has to be for this to
work?
[...]
> In commit 1234abcd (the subject line, 2016-01-01, v2.11.0), we did
> blah blah blah
The issue with this is that it is ambiguous about what the tag name is
referring to: does that mean that "git describe" and "git version"
tell me that v2.11.0 is the nearest *previous* release to that commit
or that "git name-rev" tells me that v2.11.0 is a nearby *subsequent*
release that contains it?
Of course the latter is the only answer that's useful, but in practice
the former is what people tend to do when they are trying to follow a
convention like this. So we'd need some automatic linting to make it
useful.
I think a more promising approach is the Fixes trailer Duy mentioned,
which has been working well for the Linux kernel project. I'll follow
up in a reply to his message.
Thanks,
Jonathan
^ permalink raw reply
* Re: Referring to commits in commit messages
From: Jeff King @ 2018-12-19 18:22 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jonathan Nieder, git, Han-Wen Nienhuys, Johannes Schindelin
In-Reply-To: <877eg5fwd5.fsf@evledraar.gmail.com>
On Wed, Dec 19, 2018 at 03:02:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Dec 17 2018, Jonathan Nieder wrote:
>
> > v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)
>
> Minor nit not just on this patch, but your patches in general: I think
> you're the only one using this type of template instead of the `%h
> ("%s", %ad)` format documented in SubmittingPatches.
>
> I've had at least a couple of cases where I've git log --grep=<abbr sha>
> and missed a commit of yours when you referred to another commit.
>
> E.g. when composing
> https://public-inbox.org/git/878t0lfwrj.fsf@evledraar.gmail.com/ I
> remembered PERLLIB_EXTRA went back & forth between
> working/breaking/working with your/my/your patch, so:
>
> git log --grep=0386dd37b1
>
> Just found the chain up to my breaking change, but not your 7a7bfc7adc,
> which refers to that commit as v1.9-rc0~88^2.
>
> Maybe this is really a feature request. I.e. maybe we should have some
> mode where --grep=<commitish> will be combined with some mode where we
> try to find various forms of <commitish> in commit messages, then
> normalize & match them....
That would help when you're using --grep, but not other things that are
trying to parse the commit message. Two instances I've noticed:
- web interfaces like GitHub won't linkify this type of reference
(whereas they will for something that looks like a hex object id)
- my terminal makes it easy to select hex strings, but doesn't
understand this git-describe output :)
These tools _could_ be taught a regex like /v(\d+.)(-rc\d+)?([~^]+d)*/.
But matching hex strings is a lot simpler, and works across many
projects.
So I agree with you that this git-describe format is less convenient for
readers, but my preferred solution is to use a different format, rather
than try to teach every reading tool to be more clever.
As far as I can tell, the main advantage of using "v2.11.0-rc3~3^2~1"
over its hex id is that it gives a better sense in time of which Git
version we're talking about. The date in the parentheses does something
similar for wall-clock time, but it's left to the reader to map that to
a Git version if they choose.
Personally, I find the wall-clock time to be enough, since usually what
I want to know is "how ancient is this". And in the rare instance that I
care about the containing version, it's not a big deal to use "git tag
--contains". If we really want to convey that information in the text,
I think it would be reasonable to say something like:
In commit 1234abcd (the subject line, 2016-01-01, v2.11.0), we did
blah blah blah
with a few simple rules:
- only mention a single version, the oldest one that contains the
commit[1]. If it's in v2.11.1, we can infer that it's in v2.12.0,
etc.
- only mention released commits; for the granularity we're talking
about here, the distinction between v2.11.0 and v2.11.0-rc3 is not
important
- if it hasn't been in a released version, don't include a version at
all.
That's probably over-engineering, and I'm perfectly fine with the
oid/subject/date format most people use. Just trying to give an option
if people really think the tag name is useful.
-Peff
[1] I usually compute the containing version with:
$ git help has
'has' is aliased to '!f() { git tag --contains "$@" | grep ^v | grep -v -- -rc | sort -V | head -1; }; f'
though I suspect it could be done these days with fewer processes
using "tag --sort".
^ permalink raw reply
* Re: Git hooks don't run while commiting in worktree via git-gui
From: Stephen P. Smith @ 2018-12-19 18:09 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Иван Могиш,
git,
Дмитрий Яковлев
In-Reply-To: <nycvar.QRO.7.76.6.1812182254250.43@tvgsbejvaqbjf.bet>
On Tuesday, December 18, 2018 2:56:43 PM MST Johannes Schindelin wrote:
> Sounds like you need https://github.com/git-for-windows/git/pull/1757
> (we do not currently have a responsive maintainer for Git GUI,
> unfortunately, otherwise this patch would have made it into an official
> Git version already).
Are you talking about the version that is shipped with git for windows or the
version that is shipped with the main (Junio's) version?
Too bad I only get bits and pieces of time to contribute
^ permalink raw reply
* Re: Referring to commits in commit messages
From: SZEDER Gábor @ 2018-12-19 17:38 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jonathan Nieder, git, Han-Wen Nienhuys, Johannes Schindelin
In-Reply-To: <877eg5fwd5.fsf@evledraar.gmail.com>
On Wed, Dec 19, 2018 at 03:02:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Dec 17 2018, Jonathan Nieder wrote:
>
> > v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)
>
> Minor nit not just on this patch, but your patches in general: I think
> you're the only one using this type of template instead of the `%h
> ("%s", %ad)` format documented in SubmittingPatches.
Or the '%h (%s, %ad)' format, which is more widely used in Git's
history, and those double quotes don't add any value whatsoever.
^ permalink raw reply
* Re: Referring to commits in commit messages
From: Duy Nguyen @ 2018-12-19 17:11 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jonathan Nieder, Git Mailing List, Han-Wen Nienhuys,
Johannes Schindelin
In-Reply-To: <877eg5fwd5.fsf@evledraar.gmail.com>
On Wed, Dec 19, 2018 at 6:04 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Mon, Dec 17 2018, Jonathan Nieder wrote:
>
> > v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)
>
> Minor nit not just on this patch, but your patches in general: I think
> you're the only one using this type of template instead of the `%h
> ("%s", %ad)` format documented in SubmittingPatches.
>
> I've had at least a couple of cases where I've git log --grep=<abbr sha>
> and missed a commit of yours when you referred to another commit.
>
> E.g. when composing
> https://public-inbox.org/git/878t0lfwrj.fsf@evledraar.gmail.com/ I
> remembered PERLLIB_EXTRA went back & forth between
> working/breaking/working with your/my/your patch, so:
>
> git log --grep=0386dd37b1
>
> Just found the chain up to my breaking change, but not your 7a7bfc7adc,
> which refers to that commit as v1.9-rc0~88^2.
>
> Maybe this is really a feature request. I.e. maybe we should have some
> mode where --grep=<commitish> will be combined with some mode where we
> try to find various forms of <commitish> in commit messages, then
> normalize & match them....
To follow email model, this sounds like a good trailer for related
commits, like In-Reply-To for email. We could even have special
trailer "Fixes" to indicate what commit is the problem that this
commit fixes.
--
Duy
^ permalink raw reply
* Re: [PATCH] log: add %S option (like --source) to log --format
From: Issac Trotts @ 2018-12-19 17:09 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Noemi Mercado
In-Reply-To: <CANdyxMzYMc2DGC8JUE7N6Ry7iOLH09SwR5OFQ64c1vtbO388FA@mail.gmail.com>
Looks like that was a flake. Everything passes now.
https://travis-ci.org/ijt/git/builds/469843833
On Wed, Dec 19, 2018 at 12:07 AM Issac Trotts <issac.trotts@gmail.com> wrote:
>
> Travis showed one failure. I'm not sure if it's a flake.
> https://travis-ci.org/ijt/git/builds/469843833. Rerunning it.
>
> On Tue, Dec 18, 2018 at 7:47 PM Issac Trotts <issac.trotts@gmail.com> wrote:
> >
> > On Tue, Dec 18, 2018 at 9:14 AM Issac Trotts <issac.trotts@gmail.com> wrote:
> > >
> > > Hi Peff, thanks for the feedback. I tried a variant of the command you
> > > showed and it yielded a seg fault:
> > > ```
> > > [ issactrotts ~/git ] ./git diff-tree -s --pretty=tformat:'%S %H %s' HEAD
> > > Segmentation fault: 11
> > > ```
> > > I'll see if I can track it down this evening.
> >
> > Okay, found it. My solution is to make %S not do anything for commands
> > other than `log` since `log` is the only one that has --source defined
> > as far as I can tell.
> >
> > I'm waiting for Travis to run and will post an updated patch if
> > everything looks good there.
> >
> > > Issac
> > >
> > > On Mon, Dec 17, 2018 at 7:59 AM Jeff King <peff@peff.net> wrote:
> > > >
> > > > On Sun, Dec 16, 2018 at 10:25:14PM -0800, Issac Trotts wrote:
> > > >
> > > > > Make it possible to write for example
> > > > >
> > > > > git log --format="%H,%S"
> > > > >
> > > > > where the %S at the end is a new placeholder that prints out the ref
> > > > > (tag/branch) for each commit.
> > > >
> > > > Seems like a reasonable thing to want.
> > > >
> > > > One curious thing about "--source" is that it requires cooperation from
> > > > the actual traversal. So if you did:
> > > >
> > > > git rev-list | git diff-tree --format="%H %S"
> > > >
> > > > we don't have the %S information in the latter command. I think that's
> > > > probably acceptable as long as it does something sane when we don't have
> > > > that information (e.g., replace it with an empty string). It might also
> > > > be worth calling out in the documentation.
> > > >
> > > > -Peff
^ permalink raw reply
* [ANNOUNCE] Git Rev News edition 46
From: Christian Couder @ 2018-12-19 16:41 UTC (permalink / raw)
To: git
Cc: lwn, Junio C Hamano, Jakub Narebski, Markus Jansen,
Gabriel Alcaras, Jeff King, Johannes Schindelin,
Ævar Arnfjörð Bjarmason, Luca Milanesio,
Elijah Newren, Stefan Xenos, Stefan Beller, Nguyen Thai Ngoc Duy,
Thomas Gummerer, Eric Sunshine, Dan Fabulich, Kaartic Sivaraam,
Drew DeVault
Hi everyone,
The 46th edition of Git Rev News is now published:
https://git.github.io/rev_news/2018/12/19/edition-46/
Thanks a lot to the contributors: Johannes Schindelin, Kaartic
Sivaraam and Matt Singletary!
Enjoy,
Christian, Jakub, Markus and Gabriel.
^ permalink raw reply
* Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.
From: Jeff King @ 2018-12-19 15:59 UTC (permalink / raw)
To: Erin Dahlgren; +Cc: git, Johannes Schindelin, Junio C Hamano
In-Reply-To: <CAP_Smy14j4WK-mkqdKTKue=j7YoNjfaZVCBA-7S8xwNqX2rKhQ@mail.gmail.com>
On Tue, Dec 18, 2018 at 12:54:02PM -0800, Erin Dahlgren wrote:
> > Might it make sense to make GIT_DIR_HIT_MOUNT_POINT more like
> > GIT_DIR_HIT_CEILING currently is, rather than the other way around?
> > I.e., something like:
> >
> > case GIT_DIR_HIT_CEILING:
> > if (!nongit_ok)
> > die(...);
> > *nongit_ok = 1;
> > prefix = NULL;
> > break;
> > case GIT_DIR_HIT_MOUNT_POINT:
> > if (!nongit_ok)
> > die(...);
> > *nongit_ok = 1;
> > prefix = NULL;
> > break;
> >
> > ?
>
> After some digging, there seems to be a documented guarantee that
> "`GIT_PREFIX` is set as returned by running 'git rev-parse
> --show-prefix'". See Documentation/config/alias.txt. If the
> environment variable GIT_PREFIX is already set to something and we
> don't clear it when prefix is NULL, then the two can be out of sync.
> So that's a problem with my patch and the current handling of
> GIT_DIR_HIT_MOUNT_POINT. I'm not sure how important of a guarantee it
> is, but we should respect what's documented.
Yeah, agreed.
Another benefit of avoiding the early return is that we hit the cleanup
code at the end of the function. That saves us having to release "dir"
here. I assume we don't have to care about "gitdir" in these cases, but
hitting the cleanup code means we don't even have to think about it.
Over in:
https://public-inbox.org/git/20181219154853.GC14802@sigill.intra.peff.net/
I suggested adding more cleanup to that block, too (though I _think_ in
these cases it would also be a noop, it's again nice to not have to
worry about it).
> Side note: One of the secondary goals of my patch was to make it
> really obvious that neither the GIT_DIR_HIT_CEILING nor the
> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by
> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With
> your suggestion (and it's a fair one) I don't think that's feasible
> without more significant refactoring. Should I just settle with a
> comment that explains this?
Yeah, I think a comment would probably be sufficient. Though we could
also perhaps make the two cases (i.e., we found a repo or not) more
clear. Something like:
if (!*nongit_ok || !*nongit_ok) {
startup_info->have_repository = 1;
startup_info->prefix = prefix;
if (prefix) ...etc...
} else {
start_info->have_repository = 0;
startup_info->prefix = NULL;
setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
}
That may introduce some slight repetition, but I think it's probably
clearer to deal with the cases separately (and it saves earlier code
from make-work like setting prefix=NULL when there's no repo).
The condition would also be a lot less confusing if nongit_ok were
flipped, but I think that would be a big refactor due to the way we pass
it around.
-Peff
^ permalink raw reply
* Re: [PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow
From: Jeff King @ 2018-12-19 15:51 UTC (permalink / raw)
To: git, gitster, stolee, avarab, szeder.dev
In-Reply-To: <20181218210551.GG37614@google.com>
On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote:
> On 2018.12.18 12:35, Jeff King wrote:
> > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:
> >
> > > Add a new fuzz test for the commit graph and fix a buffer read-overflow
> > > that it discovered. Additionally, fix the Makefile instructions for
> > > building fuzzers.
> > >
> > > Changes since V3:
> > > * Improve portability of the new test functionality.
> >
> > I thought there was some question about /dev/zero, which I think is
> > in this version (I don't actually know whether there are portability
> > issues or not, but somebody did mention it).
> >
> > -Peff
>
> I've only found one reference [1] (from 1999) of OS X Server not having
> a /dev/zero. It appears to be present as of 2010 though [2].
Thanks for digging. That seems like enough to assume we should try it
and see if any macOS people complain.
I do wonder if we'll run into problems on Windows, though.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] setup: add `clear_repository_format()`
From: Jeff King @ 2018-12-19 15:48 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, brian m . carlson
In-Reply-To: <20181218072528.3870492-4-martin.agren@gmail.com>
On Tue, Dec 18, 2018 at 08:25:28AM +0100, Martin Ågren wrote:
> After we set up a `struct repository_format`, it owns various pieces of
> allocated memory. We then either use those members, because we decide we
> want to use the "candidate" repository format, or we discard the
> candidate / scratch space. In the first case, we transfer ownership of
> the memory to a few global variables. In the latter case, we just
> silently drop the struct and end up leaking memory.
>
> Introduce a function `clear_repository_format()` which frees the memory
> the struct holds on to. Call it in the code paths where we currently
> leak the memory. Also call it in the error path of
> `read_repository_format()` to clean up any partial result.
>
> For hygiene, we need to at least set the pointers that we free to NULL.
> For future-proofing, let's zero the entire struct instead. It just means
> that in the error path of `read_...()` we need to restore the error
> sentinel in the `version` field.
This seems reasonable, and I very much agree on the zero-ing (even
though it _shouldn't_ matter due to the "undefined" rule). That also
makes it safe to clear() multiple times, which is a nice property.
> +void clear_repository_format(struct repository_format *format)
> +{
> + string_list_clear(&format->unknown_extensions, 0);
> + free(format->work_tree);
> + free(format->partial_clone);
> + memset(format, 0, sizeof(*format));
> }
For the callers that actually pick the values out, I think it might be a
little less error-prone if they actually copied the strings and then
called clear_repository_format(). That avoids leaks of values that they
didn't know or care about (and the cost of an extra strdup for
repository setup is not a big deal).
Something like this on top of your patch, I guess (with the idea being
that functions which return an error would clear the format, but a
"successful" one would get returned back up the stack to
setup_git_directory_gently(), which then clears it before returning.
-- >8 --
diff --git a/setup.c b/setup.c
index babe5ea156..a5699f9ee6 100644
--- a/setup.c
+++ b/setup.c
@@ -470,6 +470,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
warning("%s", err.buf);
strbuf_release(&err);
*nongit_ok = -1;
+ clear_repository_format(candidate);
return -1;
}
die("%s", err.buf);
@@ -499,7 +500,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
}
if (candidate->work_tree) {
free(git_work_tree_cfg);
- git_work_tree_cfg = candidate->work_tree;
+ git_work_tree_cfg = xstrdup(candidate->work_tree);
inside_work_tree = -1;
}
} else {
@@ -1158,6 +1159,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
strbuf_release(&dir);
strbuf_release(&gitdir);
+ clear_repository_format(&repo_fmt);
return prefix;
}
-Peff
^ permalink raw reply related
* Re: [PATCH 2/3] setup: do not use invalid `repository_format`
From: Jeff King @ 2018-12-19 15:38 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, brian m . carlson
In-Reply-To: <20181218072528.3870492-3-martin.agren@gmail.com>
On Tue, Dec 18, 2018 at 08:25:27AM +0100, Martin Ågren wrote:
> If `read_repository_format()` encounters an error, `format->version`
> will be -1 and all other fields of `format` will be undefined. However,
> in `setup_git_directory_gently()`, we use `repo_fmt.hash_algo`
> regardless of the value of `repo_fmt.version`.
>
> This can be observed by adding this to the end of
> `read_repository_format()`:
>
> if (format->version == -1)
> format->hash_algo = 0; /* no-one should peek at this! */
>
> This causes, e.g., "git branch -m q q2 without config should succeed" in
> t3200 to fail with "fatal: Failed to resolve HEAD as a valid ref."
> because it has moved .git/config out of the way and is now trying to use
> a bad hash algorithm.
>
> Check that `version` is non-negative before using `hash_algo`.
>
> This patch adds no tests, but do note that if we skip this patch, the
> next patch would cause existing tests to fail as outlined above.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Hmm. It looks like we never set repo_fmt.hash_algo to anything besides
GIT_HASH_SHA1 anyway. I guess the existing field is really just there in
preparation for us eventually respecting extensions.hashAlgorithm (or
whatever it's called).
Given what I said in my previous email about repos with a missing
"version" field, I wondered if this patch would be breaking config like:
[core]
# no repositoryformatversion!
[extensions]
hashAlgorithm = sha256
But I'd argue that:
1. That's pretty dumb config that we shouldn't need to support. Even
if we care about handling the missing version for historical repos,
they wouldn't be talking sha256.
2. Arguably we should not even look at extensions.* unless we see a
version >= 1. But we do process them as we parse the config file.
This is mostly an oversight, I think. We have to handle them as we
see them, because they may come out of order with respect to the
repositoryformatversion field. But we could put them into a
string_list, and then only process them after we've decided which
version we have.
So I think your patch is doing the right thing, and won't hurt any real
cases. But (of course) there are more opportunities to clean things up.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`
From: Jeff King @ 2018-12-19 15:27 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, brian m . carlson
In-Reply-To: <20181218072528.3870492-2-martin.agren@gmail.com>
On Tue, Dec 18, 2018 at 08:25:26AM +0100, Martin Ågren wrote:
> No-one looks at the return value, so we might as well drop it. It's
> still available as `format->version`.
>
> In v1 of what became commit 2cc7c2c737 ("setup: refactor repo format
> reading and verification", 2016-03-11), this function actually had
> return type "void", but that was changed in v2. Almost three years
> later, no-one has used this return value.
Hmm. If we have to pick one, I'd say that just returning a sane exit
value would be the more conventional thing to do. But looking at the
callers, many of them want to just pass the struct on to the verify
function.
That said, there is a long-standing curiosity here that we may want to
consider: read_repository_format() does not distinguish between these
three cases:
1. the config file is missing
2. the config file is present, but does not have a version field
3. the config file is malformed, or we experience an I/O error
(although I think there are still some cases that cause the config
parser to die(), which may be sufficient)
The comment in check_repository_format_gently() implies that git-init
needs (1) to be a non-error for historical reasons. We could probably
tighten this up for other callers.
I think (2) probably should be an error, but note that it makes t1300
very unhappy, since it stomps all over .git/config. I'm not sure if any
real-world cases would be affected.
Case (3) I think we probably ought to do a better job of diagnosing. So
I wonder if the rule should be:
- if we encounter a real error reading the config,
read_repository_format() should return -1. Most callers should
detect this and complain.
- otherwise, a missing config returns 0 but puts "-1" into the version
field
- possibly verify_repository_format() should issue a warning when it
sees "version == -1" and then rewrite the result into "0"
I dunno. This is one of those dark corners of the code where we appear
to do the wrong thing, but nobody seems to have noticed or cared much,
and changing it runs the risk of breaking some obscure cases. I'm not
sure if we should bite the bullet and try to address that, or just back
away slowly and pretend we never looked at it. ;)
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> I only discovered the full history after writing the patch. Had I known
> it from the beginning, maybe I'd have just skipped this step, but I was
> sufficiently disturbed by the redundant and unused return value that I
> dropped it before working on the actual meat of this series.
>
> cache.h | 7 +++----
> setup.c | 3 +--
> 2 files changed, 4 insertions(+), 6 deletions(-)
FWIW, the patch itself seems fine, and obviously doesn't make anything
worse on its own. The question is just whether we want to do more
cleanup here.
-Peff
^ permalink raw reply
* Built-in rebase doesn't execute post-checkout hook
From: Orgad Shaneh @ 2018-12-19 14:32 UTC (permalink / raw)
To: git, Johannes Schindelin
Hi,
The new built-in rebase feature is much faster than before. Thanks for that :)
One problem we found in it is that doesn't seem to execute post-checkout hook.
Please fix it.
Thanks,
- Orgad
^ permalink raw reply
* Referring to commits in commit messages
From: Ævar Arnfjörð Bjarmason @ 2018-12-19 14:02 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Han-Wen Nienhuys, Johannes Schindelin
In-Reply-To: <20181217165957.GA60293@google.com>
On Mon, Dec 17 2018, Jonathan Nieder wrote:
> v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)
Minor nit not just on this patch, but your patches in general: I think
you're the only one using this type of template instead of the `%h
("%s", %ad)` format documented in SubmittingPatches.
I've had at least a couple of cases where I've git log --grep=<abbr sha>
and missed a commit of yours when you referred to another commit.
E.g. when composing
https://public-inbox.org/git/878t0lfwrj.fsf@evledraar.gmail.com/ I
remembered PERLLIB_EXTRA went back & forth between
working/breaking/working with your/my/your patch, so:
git log --grep=0386dd37b1
Just found the chain up to my breaking change, but not your 7a7bfc7adc,
which refers to that commit as v1.9-rc0~88^2.
Maybe this is really a feature request. I.e. maybe we should have some
mode where --grep=<commitish> will be combined with some mode where we
try to find various forms of <commitish> in commit messages, then
normalize & match them....
^ permalink raw reply
* Re: Can git choose perl at runtime?
From: Ævar Arnfjörð Bjarmason @ 2018-12-19 13:53 UTC (permalink / raw)
To: Carlo Arenas; +Cc: John Passaro, git
In-Reply-To: <CAPUEspgw2xYxNQN-0_nqQrWE4jhmMN-9aHgJ8NvLDcEKTrZNAw@mail.gmail.com>
On Wed, Dec 19 2018, Carlo Arenas wrote:
> On Tue, Dec 18, 2018 at 7:29 PM John Passaro <john.a.passaro@gmail.com> wrote:
>>
>> I recently submitted my first patch using OSX and found the experience
>> frustrating, for reasons that have come up on the list before,
>> concerning git-send-email and perl dependencies that you need to be
>> root to update.
>
> you can install them somewhere else (your homedir, for example) and
> then instruct perl to look for them there by setting the PERL5LIB
> environment variable
Note that this is different. Cases I can think of:
1. You have an entirely different perl + modules somewhere. E.g. maybe
on OSX /usr/bin/perl v.s. some homebrew version of perl+CPAN. My WIP
https://public-inbox.org/git/87a7l1fx8x.fsf@evledraar.gmail.com/
addresses this.
2. You're happy with /usr/bin/perl (or whatever git is compiled with),
but miss some module(s). That's your suggestion here, but note that
in this case you usually need a compiler (E-Mail SSL libs etc.),
which may not be what the user wants.
I.e. they can install a new perl+modules from their package manager
easily, but can't as easily build their own modules for a system
perl.
3. Using a /usr/bin/perl + e.g. homebrew CPAN libs via a "modules over
here" facility similar to #2 is likely to segfault (different ABI
versions).
I think we're good if we just have #1 and if people have the #2 use-case
an additional core.perlLibs config of stuff to prepend to @INC (or maybe
entirly override, least we run into the segfault case in #3).
For that last bit see also 7a7bfc7adc ("perl: treat PERLLIB_EXTRA as an
extra path again", 2018-01-02). I.e. there's the use case of "@INC
instead of" and "@INC extra".
But probably you're happy with just #1 for now....
^ 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