* [PATCH v3 00/16] Remove submodule from files-backend.c
From: Nguyễn Thái Ngọc Duy @ 2017-02-17 14:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
Ramsay Jones, Stefan Beller, novalis,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216114818.6080-1-pclouds@gmail.com>
v3 only changes 07/16 but it's kinda important because I broke
packed-refs. packed-refs' path became $GIT_DIR/packed-refs instead of
$GIT_COMMON_DIR/packed-refs and as a result the majority of refs will
disappear in linked worktrees. Interdiff
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 685ea5c14..82be3f90f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -938,9 +938,11 @@ static void files_path(struct files_ref_store *refs, struct strbuf *sb,
va_start(vap, fmt);
strbuf_vaddf(&tmp, fmt, vap);
va_end(vap);
- if (is_per_worktree_ref(tmp.buf) ||
- (skip_prefix(tmp.buf, "logs/", &ref) &&
- is_per_worktree_ref(ref)))
+ if (!strcmp(tmp.buf, "packed-refs") || !strcmp(tmp.buf, "logs"))
+ strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
+ else if (is_per_worktree_ref(tmp.buf) ||
+ (skip_prefix(tmp.buf, "logs/", &ref) &&
+ is_per_worktree_ref(ref)))
strbuf_addf(sb, "%s/%s", refs->gitdir.buf, tmp.buf);
else
strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
Nguyễn Thái Ngọc Duy (16):
refs-internal.c: make files_log_ref_write() static
files-backend: convert git_path() to strbuf_git_path()
files-backend: add files_path()
files-backend: replace *git_path*() with files_path()
refs.c: share is_per_worktree_ref() to files-backend.c
refs-internal.h: correct is_per_worktree_ref()
files-backend: remove the use of git_path()
refs.c: introduce get_main_ref_store()
refs: rename lookup_ref_store() to lookup_submodule_ref_store()
refs.c: flatten get_ref_store() a bit
refs.c: kill register_ref_store(), add register_submodule_ref_store()
refs.c: make get_main_ref_store() public and use it
path.c: move some code out of strbuf_git_path_submodule()
refs: move submodule code out of files-backend.c
files-backend: remove submodule_allowed from files_downcast()
refs: rename get_ref_store() to get_submodule_ref_store() and make it public
path.c | 34 ++---
refs.c | 144 +++++++++++----------
refs.h | 13 ++
refs/files-backend.c | 349 +++++++++++++++++++++++++++++++--------------------
refs/refs-internal.h | 28 ++---
submodule.c | 31 +++++
submodule.h | 1 +
7 files changed, 348 insertions(+), 252 deletions(-)
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH v3 03/16] files-backend: add files_path()
From: Nguyễn Thái Ngọc Duy @ 2017-02-17 14:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
Ramsay Jones, Stefan Beller, novalis,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170217140436.17336-1-pclouds@gmail.com>
This will be the replacement for both git_path() and git_path_submodule()
in this file. The idea is backend takes a git path and use that,
oblivious of submodule, linked worktrees and such.
This is the middle step towards that. Eventually the "submodule" field
in 'struct files_ref_store' should be replace by "gitdir". And a
compound ref_store is created to combine two files backends together,
one represents the shared refs in $GIT_COMMON_DIR, one per-worktree. At
that point, files_path() becomes a wrapper of strbuf_vaddf().
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
refs/files-backend.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f0c878b92..abb8a95e0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -930,6 +930,24 @@ struct files_ref_store {
/* Lock used for the main packed-refs file: */
static struct lock_file packlock;
+__attribute__((format (printf, 3, 4)))
+static void files_path(struct files_ref_store *refs, struct strbuf *sb,
+ const char *fmt, ...)
+{
+ struct strbuf tmp = STRBUF_INIT;
+ va_list vap;
+
+ va_start(vap, fmt);
+ strbuf_vaddf(&tmp, fmt, vap);
+ va_end(vap);
+ if (refs->submodule)
+ strbuf_git_path_submodule(sb, refs->submodule,
+ "%s", tmp.buf);
+ else
+ strbuf_git_path(sb, "%s", tmp.buf);
+ strbuf_release(&tmp);
+}
+
/*
* Increment the reference count of *packed_refs.
*/
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH v3 02/16] files-backend: convert git_path() to strbuf_git_path()
From: Nguyễn Thái Ngọc Duy @ 2017-02-17 14:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
Ramsay Jones, Stefan Beller, novalis,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170217140436.17336-1-pclouds@gmail.com>
git_path() and friends are going to be killed in files-backend.c in near
future. And because there's a risk with overwriting buffer in
git_path(), let's convert them all to strbuf_git_path(). We'll have
easier time killing/converting strbuf_git_path() then because we won't
have to worry about memory management again.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
refs/files-backend.c | 111 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 89 insertions(+), 22 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 75565c3aa..f0c878b92 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2169,6 +2169,8 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
static int timeout_configured = 0;
static int timeout_value = 1000;
struct packed_ref_cache *packed_ref_cache;
+ struct strbuf sb = STRBUF_INIT;
+ int ret;
files_assert_main_repository(refs, "lock_packed_refs");
@@ -2177,10 +2179,13 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
timeout_configured = 1;
}
- if (hold_lock_file_for_update_timeout(
- &packlock, git_path("packed-refs"),
- flags, timeout_value) < 0)
+ strbuf_git_path(&sb, "packed-refs");
+ ret = hold_lock_file_for_update_timeout(&packlock, sb.buf,
+ flags, timeout_value);
+ strbuf_release(&sb);
+ if (ret < 0)
return -1;
+
/*
* Get the current packed-refs while holding the lock. If the
* packed-refs file has been modified since we last read it,
@@ -2335,6 +2340,9 @@ static void try_remove_empty_parents(char *name)
for (q = p; *q; q++)
;
while (1) {
+ struct strbuf sb = STRBUF_INIT;
+ int ret;
+
while (q > p && *q != '/')
q--;
while (q > p && *(q-1) == '/')
@@ -2342,7 +2350,10 @@ static void try_remove_empty_parents(char *name)
if (q == p)
break;
*q = '\0';
- if (rmdir(git_path("%s", name)))
+ strbuf_git_path(&sb, "%s", name);
+ ret = rmdir(sb.buf);
+ strbuf_release(&sb);
+ if (ret)
break;
}
}
@@ -2431,7 +2442,11 @@ static int repack_without_refs(struct files_ref_store *refs,
return 0; /* no refname exists in packed refs */
if (lock_packed_refs(refs, 0)) {
- unable_to_lock_message(git_path("packed-refs"), errno, err);
+ struct strbuf sb = STRBUF_INIT;
+
+ strbuf_git_path(&sb, "packed-refs");
+ unable_to_lock_message(sb.buf, errno, err);
+ strbuf_release(&sb);
return -1;
}
packed = get_packed_refs(refs);
@@ -2529,8 +2544,10 @@ static int rename_tmp_log(const char *newrefname)
{
int attempts_remaining = 4;
struct strbuf path = STRBUF_INIT;
+ struct strbuf tmp_renamed_log = STRBUF_INIT;
int ret = -1;
+ strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
retry:
strbuf_reset(&path);
strbuf_git_path(&path, "logs/%s", newrefname);
@@ -2546,7 +2563,7 @@ static int rename_tmp_log(const char *newrefname)
goto out;
}
- if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
+ if (rename(tmp_renamed_log.buf, path.buf)) {
if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {
/*
* rename(a, b) when b is an existing
@@ -2574,6 +2591,7 @@ static int rename_tmp_log(const char *newrefname)
ret = 0;
out:
strbuf_release(&path);
+ strbuf_release(&tmp_renamed_log);
return ret;
}
@@ -2614,9 +2632,15 @@ static int files_rename_ref(struct ref_store *ref_store,
int flag = 0, logmoved = 0;
struct ref_lock *lock;
struct stat loginfo;
- int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
+ struct strbuf sb_oldref = STRBUF_INIT;
+ struct strbuf sb_newref = STRBUF_INIT;
+ struct strbuf tmp_renamed_log = STRBUF_INIT;
+ int log, ret;
struct strbuf err = STRBUF_INIT;
+ strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+ log = !lstat(sb_oldref.buf, &loginfo);
+ strbuf_release(&sb_oldref);
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
@@ -2630,7 +2654,12 @@ static int files_rename_ref(struct ref_store *ref_store,
if (!rename_ref_available(oldrefname, newrefname))
return 1;
- if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
+ strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+ strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
+ ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
+ strbuf_release(&sb_oldref);
+ strbuf_release(&tmp_renamed_log);
+ if (ret)
return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
@@ -2709,13 +2738,19 @@ static int files_rename_ref(struct ref_store *ref_store,
log_all_ref_updates = flag;
rollbacklog:
- if (logmoved && rename(git_path("logs/%s", newrefname), git_path("logs/%s", oldrefname)))
+ strbuf_git_path(&sb_newref, "logs/%s", newrefname);
+ strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+ if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
error("unable to restore logfile %s from %s: %s",
oldrefname, newrefname, strerror(errno));
+ strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
if (!logmoved && log &&
- rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
+ rename(tmp_renamed_log.buf, sb_oldref.buf))
error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
+ strbuf_release(&sb_newref);
+ strbuf_release(&sb_oldref);
+ strbuf_release(&tmp_renamed_log);
return 1;
}
@@ -3111,22 +3146,32 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
static int files_reflog_exists(struct ref_store *ref_store,
const char *refname)
{
+ struct strbuf sb = STRBUF_INIT;
struct stat st;
+ int ret;
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "reflog_exists");
- return !lstat(git_path("logs/%s", refname), &st) &&
- S_ISREG(st.st_mode);
+ strbuf_git_path(&sb, "logs/%s", refname);
+ ret = !lstat(sb.buf, &st) && S_ISREG(st.st_mode);
+ strbuf_release(&sb);
+ return ret;
}
static int files_delete_reflog(struct ref_store *ref_store,
const char *refname)
{
+ struct strbuf sb = STRBUF_INIT;
+ int ret;
+
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "delete_reflog");
- return remove_path(git_path("logs/%s", refname));
+ strbuf_git_path(&sb, "logs/%s", refname);
+ ret = remove_path(sb.buf);
+ strbuf_release(&sb);
+ return ret;
}
static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cb_data)
@@ -3181,7 +3226,9 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "for_each_reflog_ent_reverse");
- logfp = fopen(git_path("logs/%s", refname), "r");
+ strbuf_git_path(&sb, "logs/%s", refname);
+ logfp = fopen(sb.buf, "r");
+ strbuf_release(&sb);
if (!logfp)
return -1;
@@ -3287,7 +3334,9 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "for_each_reflog_ent");
- logfp = fopen(git_path("logs/%s", refname), "r");
+ strbuf_git_path(&sb, "logs/%s", refname);
+ logfp = fopen(sb.buf, "r");
+ strbuf_release(&sb);
if (!logfp)
return -1;
@@ -3369,12 +3418,15 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
{
struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
struct ref_iterator *ref_iterator = &iter->base;
+ struct strbuf sb = STRBUF_INIT;
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "reflog_iterator_begin");
base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
- iter->dir_iterator = dir_iterator_begin(git_path("logs"));
+ strbuf_git_path(&sb, "logs");
+ iter->dir_iterator = dir_iterator_begin(sb.buf);
+ strbuf_release(&sb);
return ref_iterator;
}
@@ -3843,8 +3895,13 @@ static int files_transaction_commit(struct ref_store *ref_store,
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
- for_each_string_list_item(ref_to_delete, &refs_to_delete)
- unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
+ for_each_string_list_item(ref_to_delete, &refs_to_delete) {
+ struct strbuf sb = STRBUF_INIT;
+
+ strbuf_git_path(&sb, "logs/%s", ref_to_delete->string);
+ unlink_or_warn(sb.buf);
+ strbuf_release(&sb);
+ }
clear_loose_ref_cache(refs);
cleanup:
@@ -4098,18 +4155,28 @@ static int files_reflog_expire(struct ref_store *ref_store,
static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
{
+ struct strbuf sb = STRBUF_INIT;
+
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "init_db");
/*
* Create .git/refs/{heads,tags}
*/
- safe_create_dir(git_path("refs/heads"), 1);
- safe_create_dir(git_path("refs/tags"), 1);
+ strbuf_git_path(&sb, "refs/heads");
+ safe_create_dir(sb.buf, 1);
+ strbuf_reset(&sb);
+ strbuf_git_path(&sb, "refs/tags");
+ safe_create_dir(sb.buf, 1);
+ strbuf_reset(&sb);
if (get_shared_repository()) {
- adjust_shared_perm(git_path("refs/heads"));
- adjust_shared_perm(git_path("refs/tags"));
+ strbuf_git_path(&sb, "refs/heads");
+ adjust_shared_perm(sb.buf);
+ strbuf_reset(&sb);
+ strbuf_git_path(&sb, "refs/tags");
+ adjust_shared_perm(sb.buf);
}
+ strbuf_release(&sb);
return 0;
}
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH v3 01/16] refs-internal.c: make files_log_ref_write() static
From: Nguyễn Thái Ngọc Duy @ 2017-02-17 14:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
Ramsay Jones, Stefan Beller, novalis,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170217140436.17336-1-pclouds@gmail.com>
Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10)
but probably never used outside refs-internal.c
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
refs/files-backend.c | 3 +++
refs/refs-internal.h | 4 ----
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index cdb6b8ff5..75565c3aa 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -165,6 +165,9 @@ static struct ref_entry *create_dir_entry(struct files_ref_store *ref_store,
const char *dirname, size_t len,
int incomplete);
static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
+static int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
+ const unsigned char *new_sha1, const char *msg,
+ int flags, struct strbuf *err);
static struct ref_dir *get_ref_dir(struct ref_entry *entry)
{
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 33adbf93b..59e65958a 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -222,10 +222,6 @@ struct ref_transaction {
enum ref_transaction_state state;
};
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
- const unsigned char *new_sha1, const char *msg,
- int flags, struct strbuf *err);
-
/*
* Check for entries in extras that are within the specified
* directory, where dirname is a reference directory name including
--
2.11.0.157.gd943d85
^ permalink raw reply related
* Re: git alias for options
From: Ævar Arnfjörð Bjarmason @ 2017-02-17 13:50 UTC (permalink / raw)
To: hIpPy; +Cc: Git Mailing List
In-Reply-To: <CAM_JFCz+9mxp37BTT7XPJ0fMd41DdbAxnvQF7id9msH+SDe6_Q@mail.gmail.com>
On Fri, Feb 17, 2017 at 9:23 AM, hIpPy <hippy2981@gmail.com> wrote:
> Git has aliases for git commands. Is there a (an inbuilt) way to alias
> options? If not, what is the reason?
This has long been on my wishlist, because there's a lot of
copy/pasted logic all over Git to make git foo --whatever aliased to
foo.whatever in the config, but only for some options.
It should ideally be part of something every option just supports, via
the getopts struct.
However, it can't allow you to modify whatever option you want,
because some things like git-commit-tree should not be customized
based on config, it would break things that rely on the plumbing
commands.
So it would have to be a whitelist for each option we allow to be
configured like this via the getopts struct.
Also there are surely other edge cases, like maybe the config option
now doesn't 1=1 map to the name for the option in some cases, or the
flag should be config-able but is has no long form (which we'd like
for the config), then we'd want to add that etc.
^ permalink raw reply
* Re: body-CC-comment regression
From: Matthieu Moy @ 2017-02-17 13:16 UTC (permalink / raw)
To: Johan Hovold; +Cc: git, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger
In-Reply-To: <20170217110642.GD2625@localhost>
Johan Hovold <johan@kernel.org> writes:
> There is another option, namely to only accept a single address for tags
> in the body. I understand that being able to copy a CC-header to either
> the header section or to the command line could be useful, but I don't
> really see the point in allowing this in the tags in the body (a SoB
> always has one address, and so should a CC-tag).
I mostly agree for the SoB, but why should a Cc tag have only one email?
The "multiple emails per Cc: field" has been there for a while already
(b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got
used to it. What you are proposing breaks their flow.
> And since this is a regression for something that has been working for
> years that was introduced by a new feature, I also think it's reasonable
> to (partially) revert the feature.
I'd find it rather ironic to fix your case by breaking a feature that
has been working for more than a year :-(. What would you answer to a
contributor comming one year from now and proposing to revert your
reversion because it breaks his flow?
All that said, I think another fix would be both satisfactory for
everyone and rather simple:
1) Stop calling Mail::Address even if available. It used to make sense
to do that when our in-house parser was really poor, but we now have
something essentially as good as Mail::Address. We test our parser
against Mail::Address and we do have a few known differences (see
t9000), but they are really corner-cases and shouldn't matter.
A good consequence of this is that we stop depending on the way Perl
is installed to parse emails. Regardless of the current issue, I
think it is a good thing.
2) Modify our in-house parser to discard garbage after the >. The patch
should look like (untested):
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -903,11 +903,11 @@ sub parse_mailboxes {
my (@addr_list, @phrase, @address, @comment, @buffer) = ();
foreach my $token (@tokens) {
if ($token =~ /^[,;]$/) {
- # if buffer still contains undeterminated strings
- # append it at the end of @address or @phrase
- if ($end_of_addr_seen) {
- push @phrase, @buffer;
- } else {
+ # if buffer still contains undeterminated
+ # strings append it at the end of @address,
+ # unless we already saw the closing >, in
+ # which case we discard it.
+ if (!$end_of_addr_seen) {
push @address, @buffer;
}
What do you think?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: git alias for options
From: Michael J Gruber @ 2017-02-17 13:15 UTC (permalink / raw)
To: hIpPy, git
In-Reply-To: <CAM_JFCz+9mxp37BTT7XPJ0fMd41DdbAxnvQF7id9msH+SDe6_Q@mail.gmail.com>
hIpPy venit, vidit, dixit 17.02.2017 09:23:
> Git has aliases for git commands. Is there a (an inbuilt) way to alias
> options? If not, what is the reason?
>
> Thanks,
> hippy
>
You can setup an alias for "command with options", for example:
git help s
`git s' is aliased to `status -s -b -uno .'
As you see here, this can include non-option arguments such as the
pathsepc '.'.
You cannot alias options independent of the command, though.
Michael
^ permalink raw reply
* dotfiles in git template dir are not copied
From: greg0ire @ 2017-02-17 9:31 UTC (permalink / raw)
To: git
Hi,
I noticed yesterday that dotfiles inside the directory configured in
init.templatedir are not copied when creating a new repository.
Here is the line I think is responsible for this behavior :
https://github.com/git/git/blob/master/builtin/init-db.c#L48
Is it a bug or a feature?
Steps to reproduce, provided you already have a template dir configured :
cd $(git config --path --get init.templatedir)
touch copied
touch .not_copied
cd /tmp
mkdir whatever
cd whatever
git init
ls -la .git
On my machine, the last command does not list .not_copied .
--
greg0ire
^ permalink raw reply
* Re: body-CC-comment regression
From: Johan Hovold @ 2017-02-17 11:06 UTC (permalink / raw)
To: Matthieu Moy
Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Junio C Hamano,
Larry Finger
In-Reply-To: <vpqlgt6hug6.fsf@anie.imag.fr>
On Thu, Feb 16, 2017 at 07:16:57PM +0100, Matthieu Moy wrote:
> Johan Hovold <johan@kernel.org> writes:
>
> > Hi,
> >
> > I recently noticed that after an upgrade, git-send-email (2.10.2)
> > started aborting when trying to send patches that had a linux-kernel
> > stable-tag in its body. For example,
> >
> > Cc: <stable@vger.kernel.org> # 4.4
> >
> > was now parsed as
> >
> > "stable@vger.kernel.org#4.4"
> >
> > which resulted in
> >
> > Died at /usr/libexec/git-core/git-send-email line 1332, <FIN> line 1.
>
> This has changed in e3fdbcc8e1 (parse_mailboxes: accept extra text after
> <...> address, 2016-10-13), released v2.11.0 as you noticed:
>
> > The problem with the resulting fixes that are now in 2.11.1 is that
> > git-send-email no longer discards the trailing comment but rather
> > shoves it into the name after adding some random white space:
> >
> > "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>"
> >
> > This example is based on the example from
> > Documentation/process/stable-kernel-rules.rst:
> >
> > Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
> >
> > and this format for stable-tags has been documented at least since 2009
> > and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and
> > has been supported by git since 2012 and 831a488b76e0 ("git-send-email:
> > remove garbage after email address") I believe.
> >
> > Can we please revert to the old behaviour of simply discarding such
> > comments (from body-CC:s) or at least make it configurable through a
> > configuration option?
>
> The problem is that we now accept list of emails instead of just one
> email, so it's hard to define what "comments after the email", for
> example
>
> Cc: <foo@example.com> # , <boz@example.com>
>
> Is not accepted as two emails.
>
> So, just stripping whatever comes after # before parsing the list of
> emails would change the behavior once more, and possibly break other
> user's flow. Dropping the garbage after the email while parsing is
> possible, but only when we use our in-house parser (and we currently use
> Perl's Mail::Address when available).
>
> So, a proper fix is far from obvious, and unfortunately I won't have
> time to work on that, at least not before a while.
There is another option, namely to only accept a single address for tags
in the body. I understand that being able to copy a CC-header to either
the header section or to the command line could be useful, but I don't
really see the point in allowing this in the tags in the body (a SoB
always has one address, and so should a CC-tag).
And since this is a regression for something that has been working for
years that was introduced by a new feature, I also think it's reasonable
to (partially) revert the feature.
> OTOH, the current behavior isn't that bad. It accepts the input, and
> extracts a valid email out of it. Just the display name is admitedly
> suboptimal ...
Yeah, but the display name can end up with so much noise that auto-cc is
effectively broken for people submitting kernel patches (with stable
tags) as the only way to avoid it is to suppress all bodycc.
So what I'm proposing is to revert to the earlier behaviour of only
allowing one address per body tag by simply discarding anything
after the address.
Something like the below seems to do the trick.
Thanks,
Johan
From f551b4ca9926624dc7af6c286d7cf0f97af39541 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Fri, 17 Feb 2017 11:55:47 +0100
Subject: [PATCH] send-email: only allow one address per body tag
Adding comments after a tag in the body is a common practise (e.g. in
the Linux kernel) and git-send-email has been supporting this for years
by removing any trailing cruft after the address.
After some recent changes, any trailing comment is now instead appended
to the recipient name (with some random white space inserted) resulting
in undesirable noise in the headers, for example:
CC: "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>
Revert to the earlier behaviour of discarding anything after the (first)
address in a tag while parsing the body.
Note that multiple addresses after are still allowed after a
command-line switch (and in a CC-header).
Fixes: b1c8a11c8024 ("send-email: allow multiple emails using --cc, --to
and --bcc")
Fixes: e3fdbcc8e164 ("parse_mailboxes: accept extra text after <...>
address")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
git-send-email.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 068d60b3e698..eea0a517f71b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1563,7 +1563,7 @@ foreach my $t (@files) {
# Now parse the message body
while(<$fh>) {
$message .= $_;
- if (/^(Signed-off-by|Cc): (.*)$/i) {
+ if (/^(Signed-off-by|Cc): ([^>]*>?)/i) {
chomp;
my ($what, $c) = ($1, $2);
chomp $c;
--
2.11.1
^ permalink raw reply related
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Michael Haggerty @ 2017-02-17 10:42 UTC (permalink / raw)
To: Jeff King; +Cc: Andreas Schwab, Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <20170217080759.2357wzdiuymcyosw@sigill.intra.peff.net>
On 02/17/2017 09:07 AM, Jeff King wrote:
> [...]
> That's similar to what I wrote earlier, but if we don't mind overwriting
> errno unconditionally, I think just:
>
> errno = EIO; /* covers ferror(), overwritten by failing fclose() */
> err |= ferror(fp);
> err |= fclose(fp);
>
> does the same thing.
True; I'd forgotten the convention that non-failing functions are
allowed to change errno. Your solution is obviously simpler and faster.
Michael
^ permalink raw reply
* Re: [PATCH v2 00/19] object_id part 6
From: Michael Haggerty @ 2017-02-17 9:55 UTC (permalink / raw)
To: brian m. carlson, git; +Cc: Jeff King
In-Reply-To: <20170214023141.842922-1-sandals@crustytoothpaste.net>
On 02/14/2017 03:31 AM, brian m. carlson wrote:
> This is another series in the continuing conversion to struct object_id.
>
> This series converts more of the builtin directory and some of the refs
> code to use struct object_id. Additionally, it implements an
> nth_packed_object_oid function which provides a struct object_id version
> of the nth_packed_object function, and a parse_oid_hex function that
> makes parsing easier.
>
> The patch to use parse_oid_hex in the refs code has been split out into
> its own patch, just because I'm wary of that code and potentially
> breaking things, and I want it to be easy to revert in case things go
> wrong. I have no reason to believe it is anything other than fully
> functional, however.
>
> Changes from v1:
> * Implement parse_oid_hex and use it.
> * Make nth_packed_object_oid take a variable into which to store the
> object ID. This avoids concerns about unsafe casts.
> * Rebase on master.
Thanks as always for working on this!
I skimmed over the patches (looking more carefully at the refs-related
ones) and left a few minor comments but didn't find anything serious.
I'm curious; what fraction of the overall convert-to-object_id campaign
do you estimate is done so far? Are you getting close to the promised
land yet?
Michael
^ permalink raw reply
* Re: [PATCH v2 15/19] refs: simplify parsing of reflog entries
From: Michael Haggerty @ 2017-02-17 9:41 UTC (permalink / raw)
To: brian m. carlson, git; +Cc: Jeff King
In-Reply-To: <20170214023141.842922-16-sandals@crustytoothpaste.net>
On 02/14/2017 03:31 AM, brian m. carlson wrote:
> The current code for reflog entries uses a lot of hard-coded constants,
> making it hard to read and modify. Use parse_oid_hex and two temporary
> variables to simplify the code and reduce the use of magic constants.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> refs/files-backend.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index d7a5fd2a7c..09227a3f63 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3117,12 +3117,14 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
> char *email_end, *message;
> unsigned long timestamp;
> int tz;
> + const char *p = sb->buf;
> + const int minlen = 2 * GIT_SHA1_HEXSZ + 3;
>
> /* old SP new SP name <email> SP time TAB msg LF */
> - if (sb->len < 83 || sb->buf[sb->len - 1] != '\n' ||
> - get_oid_hex(sb->buf, &ooid) || sb->buf[40] != ' ' ||
> - get_oid_hex(sb->buf + 41, &noid) || sb->buf[81] != ' ' ||
> - !(email_end = strchr(sb->buf + 82, '>')) ||
> + if (sb->len < minlen || sb->buf[sb->len - 1] != '\n' ||
> + parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
> + parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||
> + !(email_end = strchr(p, '>')) ||
> email_end[1] != ' ' ||
> !(timestamp = strtoul(email_end + 2, &message, 10)) ||
> !message || message[0] != ' ' ||
> @@ -3136,7 +3138,7 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
> message += 6;
> else
> message += 7;
> - return fn(&ooid, &noid, sb->buf + 82, timestamp, tz, message, cb_data);
> + return fn(&ooid, &noid, sb->buf + minlen - 1, timestamp, tz, message, cb_data);
I think `sb->buf + minlen - 1` is just `p` here, isn't it?
Also, I think instead of the initial test `sb->len < minlen`, it would
be enough to check `!sb->len` (i.e., error if it's zero to avoid a
buffer underflow in `sb->buf[sb->len - 1])`, because the
`parse_oid_hex()` calls together with the `*p++ != ' '` and
`sb->buf[sb->len - 1] != '\n'` checks already ensure that the string has
at least the minimum length.
Then you could do away with `minlen` entirely.
Michael
^ permalink raw reply
* Re: topological index field for commit objects
From: Jakub Narębski @ 2017-02-17 9:28 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20170217092616.ulassn3472stbfga@sigill.intra.peff.net>
On 17 February 2017 at 10:26, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 04, 2017 at 02:43:01PM +0100, Jakub Narębski wrote:
>
>> >>>> Do Git use EWAH / EWOK bitmaps for reachability analysis, or is it still
>> >>>> limited to object counting?
>> >>>
>> >>> At GitHub we are using them for --contains analysis, along with mass
>> >>> ahead/behind (e.g., as in https://github.com/gitster/git/branches). My
>> >>> plan is to send patches upstream, but they need some cleanup first.
>> >>
>> >> Ping. have you got time to clean up those patches?
>> >
>> > No, I haven't. Don't hold your breath; it's something I hope to work on
>> > in the next 6 months, not the next 6 weeks.
>>
>> Ping, Was there any progress on this front? It is now almost 6 months
>> later...
>>
>> Could those patches be made available in a "dirty" form?
>
> I just pushed them up to the "jk/ahead-behind" branch of
> https://github.com/peff/git.
>
> They were originally done on top of v1.9.1, but I forward-ported them to
> the current "master" just now. The result compiles, but I haven't really
> tested it extensively. Caveat applier.
Thanks a lot! I'll check this out.
--
Jakub Narebski
^ permalink raw reply
* Re: topological index field for commit objects
From: Jeff King @ 2017-02-17 9:26 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git Mailing List
In-Reply-To: <CANQwDwfk2k+qGtx-_RqoLKObAgyV+ebE57UAd-VXDv86HDw2vg@mail.gmail.com>
On Sat, Feb 04, 2017 at 02:43:01PM +0100, Jakub Narębski wrote:
> >>>> Do Git use EWAH / EWOK bitmaps for reachability analysis, or is it still
> >>>> limited to object counting?
> >>>
> >>> At GitHub we are using them for --contains analysis, along with mass
> >>> ahead/behind (e.g., as in https://github.com/gitster/git/branches). My
> >>> plan is to send patches upstream, but they need some cleanup first.
> >>
> >> Ping. have you got time to clean up those patches?
> >
> > No, I haven't. Don't hold your breath; it's something I hope to work on
> > in the next 6 months, not the next 6 weeks.
>
> Ping, Was there any progress on this front? It is now almost 6 months
> later...
>
> Could those patches be made available in a "dirty" form?
I just pushed them up to the "jk/ahead-behind" branch of
https://github.com/peff/git.
They were originally done on top of v1.9.1, but I forward-ported them to
the current "master" just now. The result compiles, but I haven't really
tested it extensively. Caveat applier.
-Peff
^ permalink raw reply
* Re: [PATCH v2 14/19] hex: introduce parse_oid_hex
From: Michael Haggerty @ 2017-02-17 9:26 UTC (permalink / raw)
To: brian m. carlson, git; +Cc: Jeff King
In-Reply-To: <20170214023141.842922-15-sandals@crustytoothpaste.net>
On 02/14/2017 03:31 AM, brian m. carlson wrote:
> Introduce a function, parse_oid_hex, which parses a hexadecimal object
> ID and if successful, sets a pointer to just beyond the last character.
> This allows for simpler, more robust parsing without needing to
> hard-code integer values throughout the codebase.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> cache.h | 8 ++++++++
> hex.c | 8 ++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 61fc86e6d7..5dc89a058c 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1319,6 +1319,14 @@ extern char *oid_to_hex_r(char *out, const struct object_id *oid);
> extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer result! */
> extern char *oid_to_hex(const struct object_id *oid); /* same static buffer as sha1_to_hex */
>
> +/*
> + * Parse a hexadecimal object ID starting from hex, updating the pointer
> + * specified by p when parsing stops. The resulting object ID is stored in oid.
> + * Returns 0 on success. Parsing will stop on the first NUL or other invalid
> + * character.
> + */
> +extern int parse_oid_hex(const char *hex, struct object_id *oid, const char **p);
> +
I like this function. This is a convenient kind of interface to work
with. A few minor comments:
If you rename the nondescript `p` parameter to, say, `end`, its purpose
would be more transparent. Alternatively, `skip_prefix()` calls the
corresponding parameter `out`.
It would be nice for the docstring to mention that the object ID must be
a full, 40-character hex string. Otherwise "Parsing will stop on the
first NUL or other invalid character" makes it sound like the function
might be satisfied with fewer than 40 characters.
Finally, you might mention the useful fact that `p` is only updated if
the function succeeds.
> [...]
Michael
^ permalink raw reply
* Re: [PATCH v2 11/19] builtin/replace: convert to struct object_id
From: Michael Haggerty @ 2017-02-17 8:44 UTC (permalink / raw)
To: brian m. carlson, git; +Cc: Jeff King
In-Reply-To: <20170214023141.842922-12-sandals@crustytoothpaste.net>
On 02/14/2017 03:31 AM, brian m. carlson wrote:
> Convert various uses of unsigned char [20] to struct object_id. Rename
> replace_object_sha1 to rename_object_oid. Finally, specify a constant
> in terms of GIT_SHA1_HEXSZ.
The new name is not rename_object_oid but rather replace_object_oid.
> [...]
Michael
^ permalink raw reply
* Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Jeff King @ 2017-02-17 8:31 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <20170217035800.13214-4-kyle@kyleam.com>
On Thu, Feb 16, 2017 at 10:58:00PM -0500, Kyle Meyer wrote:
> When the current branch is renamed, the deletion of the old ref is
> recorded in HEAD's log with an empty message. Now that delete_refs()
> accepts a reflog message, provide a more descriptive message. This
> message will not be included in the reflog of the renamed branch, but
> its log already covers the renaming event with a message of "Branch:
> renamed ...".
Right, makes sense. The code overall looks fine, though I have one
nit:
> @@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store *ref_store,
> return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
> oldrefname, strerror(errno));
>
> - if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
> + strbuf_addf(&logmsg_del, "Deleted %s", oldrefname);
We've been anything but consistent with our reflog messages in the past,
but I think the general guideline for new ones is to use "command:
action". Of course we don't _know_ the action here, because we're deep
in rename_ref().
Should we have the caller pass it in for us, as we do with delete_ref()
and update_ref()?
I see we actually already have a "logmsg" parameter. It already says
"Branch: renamed %s to %s". Could we just reuse that? I know that this
step is technically just the deletion, but I think it more accurately
describes the whole operation that the deletion is part of.
-Peff
^ permalink raw reply
* git alias for options
From: hIpPy @ 2017-02-17 8:23 UTC (permalink / raw)
To: git
Git has aliases for git commands. Is there a (an inbuilt) way to alias
options? If not, what is the reason?
Thanks,
hippy
^ permalink raw reply
* Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs
From: Jeff King @ 2017-02-17 8:22 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <20170217035800.13214-3-kyle@kyleam.com>
On Thu, Feb 16, 2017 at 10:57:59PM -0500, Kyle Meyer wrote:
> Now that delete_refs() accepts a reflog message, pass the
> user-provided message to delete_refs() rather than silently dropping
> it. The doesn't matter for the deleted ref's log because the log is
> deleted along with the ref, but this entry will show up in HEAD's
> reflog when deleting a checked out branch.
Sounds good.
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index a41f9adf1..f642acc22 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
> */
> return delete_ref(refname,
> (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
> - flags, NULL);
> + flags, msg);
This looks obviously correct.
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index b0ffc0b57..65918d984 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
> '
> rm -f .git/$m
>
> +test_expect_success "deleting current branch adds message to HEAD's log" '
> + git update-ref $m $A &&
> + git symbolic-ref HEAD $m &&
> + git update-ref -mdelmsg -d $m &&
> + ! test -f .git/$m &&
> + grep "delmsg$" .git/logs/HEAD >/dev/null
> +'
> +rm -f .git/$m
I think covering this with a test is good.
I don't know if it's also worth testing that deleting via HEAD also
writes the reflog. I.e.,:
git update-ref -m delete-by-head -d HEAD
Some of the style here is a bit out-dated, but I think you are just
matching the surrounding tests. So that's OK by me (though a patch to
modernize the whole thing would be welcome, too).
For reference, the two things I notice are:
- we prefer test_path_is_missing to "! test -f" these days.
- we don't redirect the output of grep (it's handled already in
non-verbose mode, and in verbose mode we try to be...verbose).
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] delete_ref(): support reflog messages
From: Jeff King @ 2017-02-17 8:17 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <20170217035800.13214-1-kyle@kyleam.com>
On Thu, Feb 16, 2017 at 10:57:57PM -0500, Kyle Meyer wrote:
> [Sorry for the slow response.]
No problem. The pace of open source varies wildly. :)
> > - "git branch -m" does seem to realize when we are renaming HEAD,
> > because it updates HEAD to point to the new branch name. But it
> > should probably insert another reflog entry mentioning the rename
> > (we do for "git checkout foo", even when "foo" has the same sha1 as
> > the current HEAD).
>
> I haven't worked out how to do this part yet. I'm guessing the change
> will involve modifying split_head_update().
>
> If this is added, should it be instead of, rather than in addition to,
> the deletion entry? If a "Branch: renamed ..." entry is present, it
> doesn't seem like the deletion entry is providing any additional
> information.
I think you could do an "instead of" that goes from sha1 X to X, and
just mentions the rename. Or you could add a second entry after the
delete that takes it from 0{40} back to X.
I suspect the latter is easier to do, and I doubt anybody would care
that much of the exact form. These entries aren't really doing anything
for reachability. They're just giving an audit log of what happened. So
I don't think anybody would really care unless they were debugging a
confusing situation by hand. And as long as there's enough information
to figure out what happened, they'll be happy.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] delete_refs(): accept a reflog message argument
From: Jeff King @ 2017-02-17 8:12 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <20170217035800.13214-2-kyle@kyleam.com>
On Thu, Feb 16, 2017 at 10:57:58PM -0500, Kyle Meyer wrote:
> When the current branch is renamed with 'git branch -m/-M' or deleted
> with 'git update-ref -m<msg> -d', the event is recorded in HEAD's log
> with an empty message.
>
> In preparation for adding a more meaningful message to HEAD's log in
> these cases, update delete_ref() to take a message argument and pass
> it along to ref_transaction_delete(). Modify all callers to pass NULL
> for the new message argument; no change in behavior is intended.
Seems like a good first step.
> diff --git a/refs.h b/refs.h
> index 9fbff90e7..81627a63d 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
> * be NULL_SHA1. flags is passed through to ref_transaction_delete().
> */
> int delete_ref(const char *refname, const unsigned char *old_sha1,
> - unsigned int flags);
> + unsigned int flags, const char *msg);
Should the "msg" argument go at the beginning, to match update_ref()?
-Peff
^ permalink raw reply
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Jeff King @ 2017-02-17 8:07 UTC (permalink / raw)
To: Michael Haggerty
Cc: Andreas Schwab, Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <923e328c-7fea-a9e4-1059-3bd6b8e58164@alum.mit.edu>
On Fri, Feb 17, 2017 at 09:00:09AM +0100, Michael Haggerty wrote:
> As you pointed out, if ferror() fails, it doesn't set errno properly. At
> least one caller tries to strerror(errno), so it would probably be good
> to store *something* in there, probably EIO.
Yeah, we discussed this up-thread a bit, and my "solution" was similar
to yours. I don't like it, because EIO is a real thing that can happen,
too, and it would certainly be surprising to a user to see. But it's
probably better than the alternative, which is getting whatever random
value happened to be in errno.
The only downside is that if the value of errno _was_ valid (because the
last thing you did really was writing to the filehandle, then we'd
overwrite it).
> To be really pedantic, there's also the question of what errno the
> caller would want if *both* ferror() and fclose() fail. Normally I would
> say "the first error that occurred", but in this case we don't know the
> correct errno from the error reported by ferror(), so maybe the fclose()
> errno is more likely to hint at the underlying reason for the failure.
Yes, I think we're better to take what fclose gives us, if we can.
> So I (reluctantly) propose
>
> if (ferror(fp)) {
> if (!fclose(fp)) {
> /*
> * ferror() doesn't set errno, so we have to
> * set one. (By contrast, when fclose() fails
> * too, we leave *its* errno in place.)
> */
> errno = EIO;
> }
> return -1;
> }
> return fclose();
That's similar to what I wrote earlier, but if we don't mind overwriting
errno unconditionally, I think just:
errno = EIO; /* covers ferror(), overwritten by failing fclose() */
err |= ferror(fp);
err |= fclose(fp);
does the same thing.
-Peff
^ permalink raw reply
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Michael Haggerty @ 2017-02-17 8:00 UTC (permalink / raw)
To: Jeff King, Andreas Schwab; +Cc: Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <20170216213140.xqw7gzjimhvg7tcm@sigill.intra.peff.net>
On 02/16/2017 10:31 PM, Jeff King wrote:
> On Thu, Feb 16, 2017 at 11:43:59AM -0500, Jeff King wrote:
>
>> On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:
>>
>>>>> int xfclose(FILE *fp)
>>>>> {
>>>>> return ferror(fp) | fclose(fp);
>>>>> }
>>>>
>>>> Yes, that's exactly what I had in mind (might be worth calling out the
>>>> bitwise-OR, though, just to make it clear it's not a typo).
>>>
>>> Since the order of evaluation is unspecified, it would be better to
>>> force sequencing ferror before fclose.
>>
>> Good point. Arguably the call in tempfile.c is buggy.
>
> Here's a fix.
>
> I think close_tempfile() suffers from the same errno problem discussed
> earlier in this thread (i.e., that after calling it, you may get an
> error return with a random, unrelated errno value if ferror() failed but
> fclose() did not).
>
> -- >8 --
> Subject: [PATCH] tempfile: avoid "ferror | fclose" trick
>
> The current code wants to record an error condition from
> either ferror() or fclose(), but makes sure that we always
> call both functions. So it can't use logical-OR "||", which
> would short-circuit when ferror() is true. Instead, it uses
> bitwise-OR "|" to evaluate both functions and set one or
> more bits in the "err" flag if they reported a failure.
>
> Unlike logical-OR, though, bitwise-OR does not introduce a
> sequence point, and the order of evaluation for its operands
> is unspecified. So a compiler would be free to generate code
> which calls fclose() first, and then ferror() on the
> now-freed filehandle.
>
> There's no indication that this has happened in practice,
> but let's write it out in a way that follows the standard.
>
> Noticed-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> tempfile.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/tempfile.c b/tempfile.c
> index 2990c9242..ffcc27237 100644
> --- a/tempfile.c
> +++ b/tempfile.c
> @@ -247,12 +247,8 @@ int close_tempfile(struct tempfile *tempfile)
> tempfile->fd = -1;
> if (fp) {
> tempfile->fp = NULL;
> -
> - /*
> - * Note: no short-circuiting here; we want to fclose()
> - * in any case!
> - */
> - err = ferror(fp) | fclose(fp);
> + err = ferror(fp);
> + err |= fclose(fp);
> } else {
> err = close(fd);
> }
>
Thanks for fixing this; the old code was definitely wrong.
As you pointed out, if ferror() fails, it doesn't set errno properly. At
least one caller tries to strerror(errno), so it would probably be good
to store *something* in there, probably EIO.
To be really pedantic, there's also the question of what errno the
caller would want if *both* ferror() and fclose() fail. Normally I would
say "the first error that occurred", but in this case we don't know the
correct errno from the error reported by ferror(), so maybe the fclose()
errno is more likely to hint at the underlying reason for the failure.
So I (reluctantly) propose
if (ferror(fp)) {
if (!fclose(fp)) {
/*
* ferror() doesn't set errno, so we have to
* set one. (By contrast, when fclose() fails
* too, we leave *its* errno in place.)
*/
errno = EIO;
}
return -1;
}
return fclose();
Michael
^ permalink raw reply
* Re: difflame improvements
From: Edmundo Carmona Antoranz @ 2017-02-17 7:01 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <20170217051755.fx2ueizsprw2vida@sigill.intra.peff.net>
On Thu, Feb 16, 2017 at 11:17 PM, Jeff King <peff@peff.net> wrote:
> This isn't difflame's fault; that's what "git blame" tells you about
> that line. But since I already told difflame "v2.6.5..HEAD", it would
> probably make sense to similarly limit the blame to that range. That
> turns up a boundary commit for the line. Which is _also_ not helpful,
> but at least the tool is telling me that the line came from before
> v2.6.5, and I don't really need to care much about it.
I'm running my own tests on difflame and I have a theory about "when"
it breaks.... at least one of the cases when it breaks:
Analysis for deleted lines is being driven by git blame --reverse.
What I have noticed is that it "breaks" when blame --reverse drives
the analysis into revisions where "treeish1" is not part of their
history (like, bringing analysis "to the sides" of treeish1 instead of
keeping analysis in revisions in the history of treeish2 that have
treeish1 as one of their ancestors.... which is definitely a valid
case for analysis, anyway). In this case, blame --reverse stops being
helpful.
Take this example (I just pushed a debug-deletion branch into gh...
probably more debugging messages will be needed):
$ difflame.py HEAD~100 HEAD -- Documentation/git-commit.txt
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f2ab0ee2e..4f8f20a36 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -265,7 +265,8 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
bcf9626a71 (Matthieu Moy 2016-06-28 13:40:11 +0200 265) If this
option is specified together with `--amend`, then
04c8ce9c1c (Markus Heidelberg 2008-12-19 13:14:18 +0100 266) no
paths need to be specified, which can be used to amend
d4ba07cac5 (Johannes Sixt 2008-04-10 13:33:09 +0200 267) the
last commit without committing changes that have
Range of revisions: 02db2d..066fb04
Treeish1 02db2d04: 02db2d042 Merge branch 'ah/grammos'
Treeish2 066fb0494: 066fb0494 blame: draft of line format
Blamed Revision afe0e2a39: afe0e2a39 Merge branch
'da/difftool-dir-diff-fix'
Original Filename a/Documentation/git-commit.txt Deleted Line 268
Children revisions:
3aead1cad7a: 3aead1cad Merge branch 'ak/commit-only-allow-empty'
There's only one child revision.... on that revision the line
we are tracking is gone
Parents of this child revision:
afe0e2a39166: afe0e2a39 Merge branch 'da/difftool-dir-diff-fix'
beb635ca9ce: beb635ca9 commit: remove 'Clever' message
for --only --amend
Finding parent where the line has been deleted:
beb635ca9: beb635ca9 commit: remove 'Clever' message
for --only --amend
Range of revisions: 02db2d042..beb635c
Treeish1 02db2d0: 02db2d042 Merge branch 'ah/grammos'
Treeish2 beb635c: beb635ca9 commit: remove 'Clever'
message for --only --amend
Blamed Revision 02db2d0: 02db2d042 Merge branch 'ah/grammos'
Original Filename a/Documentation/git-commit.txt Deleted Line 268
Children revisions:
Found no children... will return the original blamed revision
(02db2d0) saying that the deleting revision could not be found
beb635ca9 commit: remove 'Clever' message for --only --amend
-beb635ca9 (Andreas Krey 2016-12-09 05:10:21 +0100 268)
already been staged.
319d83524 commit: make --only --allow-empty work without paths
+319d835240 (Andreas Krey 2016-12-02 23:15:13 +0100 268)
already been staged. ...
+319d835240 (Andreas Krey 2016-12-02 23:15:13 +0100 269) paths
are also not requi...
d4ba07cac5 (Johannes Sixt 2008-04-10 13:33:09 +0200 270)
1947bdbc31 (Junio C Hamano 2008-06-22 14:32:27 -0700 271) -u[<mode>]::
1947bdbc31 (Junio C Hamano 2008-06-22 14:32:27 -0700 272)
--untracked-files[=<mode>]::
I know that line 268 was deleted on 319d835240.
So.... on the first round of merge analysis it says "let's go into
beb635ca9". That's fine. That's exactly the path that is required to
reach 319d835240. But then when using this new "range of revisions"
for git blame --reverse, we get that line 268 is not telling us
anything useful:
$ git blame --reverse -L268,268 02db2d042..beb635c --
Documentation/git-commit.txt
^02db2d042 (Junio C Hamano 2016-12-19 14:45:30 -0800 268)
already been staged.
So, instead of pointing to 319d835240 (the parent of beb635c), it's
basically saying something like "I give up". My hunch (haven't sat
down to digest all the details about the output of git blame
--reverse... YET) is that, given that 02db2d042 is _not_ part of the
history of beb635c, git blame reverse is trying to tell me just
that... and that means I'll have to "script around this scenario".
$ git merge-base 02db2d042 beb635c
0202c411edc25940cc381bf317badcdf67670be4
Thanks in advance.
^ permalink raw reply related
* Re: difflame improvements
From: Jeff King @ 2017-02-17 5:17 UTC (permalink / raw)
To: Edmundo Carmona Antoranz; +Cc: Git List
In-Reply-To: <CAOc6etYz6+FzDRbkLS2SB9=F9DP18=6uLKdfCN3D3yd2Gug-tw@mail.gmail.com>
On Tue, Feb 14, 2017 at 11:19:05PM -0600, Edmundo Carmona Antoranz wrote:
> I've been working on detecting revisions where a "real" deletion was
> made and I think I advanced a lot in that front. I still have to work
> on many scenarios (renamed files, for example... also performance) but
> at least I'm using a few runs against git-scm history and the results
> are "promising":
I played with this a bit more, and it did turn up the correct results
for some deletions in my experiments.
One thing I noticed is that it also turned up nonsense for lines that
blame in weird ways. For instance, I have a diff like this (these are
real examples, but don't pay attention to the sha1s; it's in a fork of
git, not upstream):
$ git diff v2.6.5 builtin/prune-packed.c
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index 7cf900ea07..5e3727e841 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -2,6 +2,7 @@
#include "cache.h"
#include "progress.h"
#include "parse-options.h"
+#include "gh-log.h"
static const char * const prune_packed_usage[] = {
N_("git prune-packed [-n | --dry-run] [-q | --quiet]"),
@@ -29,8 +30,11 @@ static int prune_object(const unsigned char *sha1, const char *path,
if (*opts & PRUNE_PACKED_DRY_RUN)
printf("rm -f %s\n", path);
- else
+ else {
+ gh_logf("prune", "%s Duplicate loose object pruned\n",
+ sha1_to_hex(sha1));
unlink_or_warn(path);
+ }
return 0;
}
Running difflame on it says this:
$ python /path/to/difflame.py v2.6.5..HEAD -- builtin/prune-packed.c
[...]
-2c0b29e662 (Jeff King 2016-01-26 15:27:55 -0500 32) else
+d60032f8640 builtin/prune-packed.c (Jeff King 2015-02-02 23:15:33 -0500 33) else {
+d60032f8640 builtin/prune-packed.c (Jeff King 2015-02-02 23:15:33 -0500 34) gh_logf("prune", "%s Duplicate loose object pruned\n",
+d60032f8640 builtin/prune-packed.c (Jeff King 2015-02-02 23:15:33 -0500 35) sha1_to_hex(sha1));
0d3b729680e builtin/prune-packed.c (Jeff King 2014-10-15 18:40:53 -0400 36) unlink_or_warn(path);
+2396ec85bd1 prune-packed.c (Linus Torvalds 2005-07-03 14:27:34 -0700 37) }
There are two weird things. One is that the old "else" is attributed to
my 2c0b29e662. That's quite weird, because that is a merge commit which
did not touch the file at all. I haven't tracked it down, but presumably
that is weirdness with the --reverse blame.
But there's another one, that I think is easy to fix. The closing brace
is attributed to some ancient commit from Linus. Which yes, I'm sure had
a closing brace, but not _my_ closing brace that was added by
d60032f8640, that the rest of the lines got attributed to.
This isn't difflame's fault; that's what "git blame" tells you about
that line. But since I already told difflame "v2.6.5..HEAD", it would
probably make sense to similarly limit the blame to that range. That
turns up a boundary commit for the line. Which is _also_ not helpful,
but at least the tool is telling me that the line came from before
v2.6.5, and I don't really need to care much about it.
Part of this is that my use case may be a bit different than yours. I
don't actually want to look at the blame results directly. I just want
to see the set of commits that I'd need to look at and possibly
cherry-pick in order to re-create the diff.
-Peff
^ 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