* [PATCH v4 16/23] log_ref_setup(): manage the name of the reflog file internally
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
Instead of writing the name of the reflog file into a strbuf that is
supplied by the caller but not needed there, write it into a local
temporary buffer and remove the strbuf parameter entirely.
And while we're adjusting the function signature, reorder the arguments
to move the input parameters before the output parameters.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 69 ++++++++++++++++++++++++++--------------------------
1 file changed, 34 insertions(+), 35 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9c5e804..846380f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2719,37 +2719,36 @@ static int open_or_create_logfile(const char *path, void *cb)
}
/*
- * Create a reflog for a ref. Store its path to *logfile. If
- * force_create = 0, only create the reflog for certain refs (those
- * for which should_autocreate_reflog returns non-zero). Otherwise,
- * create it regardless of the reference name. If the logfile already
- * existed or was created, return 0 and set *logfd to the file
- * descriptor opened for appending to the file. If no logfile exists
- * and we decided not to create one, return 0 and set *logfd to -1. On
- * failure, fill in *err, set *logfd to -1, and return -1.
+ * Create a reflog for a ref. If force_create = 0, only create the
+ * reflog for certain refs (those for which should_autocreate_reflog
+ * returns non-zero). Otherwise, create it regardless of the reference
+ * name. If the logfile already existed or was created, return 0 and
+ * set *logfd to the file descriptor opened for appending to the file.
+ * If no logfile exists and we decided not to create one, return 0 and
+ * set *logfd to -1. On failure, fill in *err, set *logfd to -1, and
+ * return -1.
*/
-static int log_ref_setup(const char *refname,
- struct strbuf *logfile, int *logfd,
- struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname, int force_create,
+ int *logfd, struct strbuf *err)
{
- strbuf_git_path(logfile, "logs/%s", refname);
+ char *logfile = git_pathdup("logs/%s", refname);
if (force_create || should_autocreate_reflog(refname)) {
- if (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd)) {
+ if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
if (errno == ENOENT)
strbuf_addf(err, "unable to create directory for '%s': "
- "%s", logfile->buf, strerror(errno));
+ "%s", logfile, strerror(errno));
else if (errno == EISDIR)
strbuf_addf(err, "there are still logs under '%s'",
- logfile->buf);
+ logfile);
else
strbuf_addf(err, "unable to append to '%s': %s",
- logfile->buf, strerror(errno));
+ logfile, strerror(errno));
- return -1;
+ goto error;
}
} else {
- *logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
+ *logfd = open(logfile, O_APPEND | O_WRONLY, 0666);
if (*logfd < 0) {
if (errno == ENOENT || errno == EISDIR) {
/*
@@ -2761,34 +2760,39 @@ static int log_ref_setup(const char *refname,
;
} else {
strbuf_addf(err, "unable to append to '%s': %s",
- logfile->buf, strerror(errno));
- return -1;
+ logfile, strerror(errno));
+ goto error;
}
}
}
if (*logfd >= 0)
- adjust_shared_perm(logfile->buf);
+ adjust_shared_perm(logfile);
+ free(logfile);
return 0;
+
+error:
+ free(logfile);
+ return -1;
}
static int files_create_reflog(struct ref_store *ref_store,
const char *refname, int force_create,
struct strbuf *err)
{
- int ret;
- struct strbuf sb = STRBUF_INIT;
int fd;
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "create_reflog");
- ret = log_ref_setup(refname, &sb, &fd, err, force_create);
+ if (log_ref_setup(refname, force_create, &fd, err))
+ return -1;
+
if (fd >= 0)
close(fd);
- strbuf_release(&sb);
- return ret;
+
+ return 0;
}
static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
@@ -2819,16 +2823,15 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg,
- struct strbuf *logfile, int flags,
- struct strbuf *err)
+ int flags, struct strbuf *err)
{
int logfd, result;
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
- result = log_ref_setup(refname, logfile, &logfd, err,
- flags & REF_FORCE_CREATE_REFLOG);
+ result = log_ref_setup(refname, flags & REF_FORCE_CREATE_REFLOG,
+ &logfd, err);
if (result)
return result;
@@ -2859,11 +2862,7 @@ 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)
{
- struct strbuf sb = STRBUF_INIT;
- int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, flags,
- err);
- strbuf_release(&sb);
- return ret;
+ return log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err);
}
/*
--
2.9.3
^ permalink raw reply related
* [PATCH v4 12/23] log_ref_setup(): separate code for create vs non-create
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
The behavior of this function (especially how it handles errors) is
quite different depending on whether we are willing to create the reflog
vs. whether we are only trying to open an existing reflog. So separate
the code paths.
This also simplifies the next steps.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 59 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 39 insertions(+), 20 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index fd8a751..c8f6d82 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2718,45 +2718,64 @@ static int commit_ref(struct ref_lock *lock)
*/
static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
{
- int logfd, oflags = O_APPEND | O_WRONLY;
+ int logfd;
strbuf_git_path(logfile, "logs/%s", refname);
+
if (force_create || should_autocreate_reflog(refname)) {
if (safe_create_leading_directories(logfile->buf) < 0) {
strbuf_addf(err, "unable to create directory for '%s': "
"%s", logfile->buf, strerror(errno));
return -1;
}
- oflags |= O_CREAT;
- }
-
- logfd = open(logfile->buf, oflags, 0666);
- if (logfd < 0) {
- if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))
- return 0;
+ logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
+ if (logfd < 0) {
+ if (errno == EISDIR) {
+ /*
+ * The directory that is in the way might be
+ * empty. Try to remove it.
+ */
+ if (remove_empty_directories(logfile)) {
+ strbuf_addf(err, "there are still logs under "
+ "'%s'", logfile->buf);
+ return -1;
+ }
+ logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
+ }
- if (errno == EISDIR) {
- if (remove_empty_directories(logfile)) {
- strbuf_addf(err, "there are still logs under "
- "'%s'", logfile->buf);
+ if (logfd < 0) {
+ strbuf_addf(err, "unable to append to '%s': %s",
+ logfile->buf, strerror(errno));
return -1;
}
- logfd = open(logfile->buf, oflags, 0666);
}
-
+ } else {
+ logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
if (logfd < 0) {
- strbuf_addf(err, "unable to append to '%s': %s",
- logfile->buf, strerror(errno));
- return -1;
+ if (errno == ENOENT || errno == EISDIR) {
+ /*
+ * The logfile doesn't already exist,
+ * but that is not an error; it only
+ * means that we won't write log
+ * entries to it.
+ */
+ ;
+ } else {
+ strbuf_addf(err, "unable to append to '%s': %s",
+ logfile->buf, strerror(errno));
+ return -1;
+ }
}
}
- adjust_shared_perm(logfile->buf);
- close(logfd);
+ if (logfd >= 0) {
+ adjust_shared_perm(logfile->buf);
+ close(logfd);
+ }
+
return 0;
}
-
static int files_create_reflog(struct ref_store *ref_store,
const char *refname, int force_create,
struct strbuf *err)
--
2.9.3
^ permalink raw reply related
* [PATCH v4 15/23] log_ref_write_1(): don't depend on logfile argument
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
It's unnecessary to pass a strbuf holding the reflog path up and down
the call stack now that it is hardly needed by the callers. Remove the
places where log_ref_write_1() uses it, in preparation for making it
internal to log_ref_setup().
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f723834..9c5e804 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2838,14 +2838,18 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
result = log_ref_write_fd(logfd, old_sha1, new_sha1,
git_committer_info(0), msg);
if (result) {
- strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
- strerror(errno));
+ int save_errno = errno;
+
+ strbuf_addf(err, "unable to append to '%s': %s",
+ git_path("logs/%s", refname), strerror(save_errno));
close(logfd);
return -1;
}
if (close(logfd)) {
- strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
- strerror(errno));
+ int save_errno = errno;
+
+ strbuf_addf(err, "unable to append to '%s': %s",
+ git_path("logs/%s", refname), strerror(save_errno));
return -1;
}
return 0;
--
2.9.3
^ permalink raw reply related
* [PATCH v4 17/23] log_ref_write_1(): inline function
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
Now files_log_ref_write() doesn't do anything beyond call
log_ref_write_1(), so inline the latter into the former.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 846380f..39d6f5b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2821,9 +2821,9 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
return 0;
}
-static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
- const unsigned char *new_sha1, const char *msg,
- int flags, struct strbuf *err)
+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)
{
int logfd, result;
@@ -2858,13 +2858,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
return 0;
}
-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)
-{
- return log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err);
-}
-
/*
* Write sha1 into the open lockfile, then close the lockfile. On
* errors, rollback the lockfile, fill in *err and
--
2.9.3
^ permalink raw reply related
* [PATCH v4 14/23] log_ref_setup(): pass the open file descriptor back to the caller
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
This function will most often be called by log_ref_write_1(), which
wants to append to the reflog file. In that case, it is silly to close
the file only for the caller to reopen it immediately. So, in the case
that the file was opened, pass the open file descriptor back to the
caller.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 39 ++++++++++++++++++++++-----------------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 27d4fd3..f723834 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2719,19 +2719,23 @@ static int open_or_create_logfile(const char *path, void *cb)
}
/*
- * Create a reflog for a ref. If force_create = 0, the reflog will
- * only be created for certain refs (those for which
- * should_autocreate_reflog returns non-zero. Otherwise, create it
- * regardless of the ref name. Fill in *err and return -1 on failure.
+ * Create a reflog for a ref. Store its path to *logfile. If
+ * force_create = 0, only create the reflog for certain refs (those
+ * for which should_autocreate_reflog returns non-zero). Otherwise,
+ * create it regardless of the reference name. If the logfile already
+ * existed or was created, return 0 and set *logfd to the file
+ * descriptor opened for appending to the file. If no logfile exists
+ * and we decided not to create one, return 0 and set *logfd to -1. On
+ * failure, fill in *err, set *logfd to -1, and return -1.
*/
-static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname,
+ struct strbuf *logfile, int *logfd,
+ struct strbuf *err, int force_create)
{
- int logfd;
-
strbuf_git_path(logfile, "logs/%s", refname);
if (force_create || should_autocreate_reflog(refname)) {
- if (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd)) {
+ if (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd)) {
if (errno == ENOENT)
strbuf_addf(err, "unable to create directory for '%s': "
"%s", logfile->buf, strerror(errno));
@@ -2745,8 +2749,8 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
return -1;
}
} else {
- logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
- if (logfd < 0) {
+ *logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
+ if (*logfd < 0) {
if (errno == ENOENT || errno == EISDIR) {
/*
* The logfile doesn't already exist,
@@ -2763,10 +2767,8 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
}
}
- if (logfd >= 0) {
+ if (*logfd >= 0)
adjust_shared_perm(logfile->buf);
- close(logfd);
- }
return 0;
}
@@ -2777,11 +2779,14 @@ static int files_create_reflog(struct ref_store *ref_store,
{
int ret;
struct strbuf sb = STRBUF_INIT;
+ int fd;
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "create_reflog");
- ret = log_ref_setup(refname, &sb, err, force_create);
+ ret = log_ref_setup(refname, &sb, &fd, err, force_create);
+ if (fd >= 0)
+ close(fd);
strbuf_release(&sb);
return ret;
}
@@ -2817,17 +2822,17 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
struct strbuf *logfile, int flags,
struct strbuf *err)
{
- int logfd, result, oflags = O_APPEND | O_WRONLY;
+ int logfd, result;
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
- result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);
+ result = log_ref_setup(refname, logfile, &logfd, err,
+ flags & REF_FORCE_CREATE_REFLOG);
if (result)
return result;
- logfd = open(logfile->buf, oflags);
if (logfd < 0)
return 0;
result = log_ref_write_fd(logfd, old_sha1, new_sha1,
--
2.9.3
^ permalink raw reply related
* git branch --editdescription fatal error
From: Jake Lambert @ 2017-01-06 17:25 UTC (permalink / raw)
To: git@vger.kernel.org
Hello,
When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place. I am not too familiar with the inner workings of git, but if you accept pull requests, I'm happy to give it a shot if you can give me directions as to how to submit one to you.
While I have you, I would also like to make a feature request for branch descriptions to (optionally) be included in the output of git branch <noargs> (or something like git branch --descriptions). This is particularly helpful where branch names match JIRA tickets numbers or are simply so old I don't remember what they are but don't want to delete them just in case.
Thank you for maintaining this absolutely wonderful program, and have a happy New Year!
Best,
Jake
Jake Lambert
Full Stack Developer
The Company On A Hill
www.onahill.co
^ permalink raw reply
* Re: git branch --editdescription fatal error
From: Stefan Beller @ 2017-01-06 17:55 UTC (permalink / raw)
To: Jake Lambert; +Cc: git@vger.kernel.org
In-Reply-To: <MWHPR19MB11357994E6C43DBCC0931CD7BC630@MWHPR19MB1135.namprd19.prod.outlook.com>
On Fri, Jan 6, 2017 at 9:25 AM, Jake Lambert <jake@onahill.co> wrote:
> Hello,
>
> When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place.
That seems strange. Is it possible that your config is not writable?
(.git/config, ~/gitconfig, you'd need to find out where the <branch>
is configured already via git config --global/--system/--local --list)
> I am not too familiar with the inner workings of git, but if you accept pull requests, I'm happy to give it a shot if you can give me directions as to how to submit one to you.
We rather do email based workflow, see
https://github.com/git/git/blob/master/Documentation/SubmittingPatches
(We use github only as a host, you could also obtain it from one of
https://git-blame.blogspot.com/p/git-public-repositories.html)
>
> While I have you, I would also like to make a feature request for branch descriptions to (optionally) be included in the output of git branch <noargs> (or something like git branch --descriptions). This is particularly helpful where branch names match JIRA tickets numbers or are simply so old I don't remember what they are but don't want to delete them just in case.
Apparently the branch descriptions are only used for the request-pull
subcommand,
https://github.com/git/git/commit/c0168147831fce00975949213eef3471b7a2b76b
and the merge message
https://github.com/git/git/commit/898eacd8ada2d012f977948350ed60845e238037
If you want to work on both of these issues, have a look at
edit_branch_description
as well as print_ref_list in builtin/branch.c
> Thank you for maintaining this absolutely wonderful program, and have a happy New Year!
Happy new year!
Stefan
^ permalink raw reply
* Rebasing a branch with merges
From: Robert Dailey @ 2017-01-06 19:12 UTC (permalink / raw)
To: Git
Here's the scenario:
I create a topic branch so one other developer and myself can work on
a feature that takes 2 weeks to complete. During that 2 week period,
changes are occurring on master that I need in my topic branch. Since
I have a collaborator on the branch, I opt for merges instead of
rebase.
Each day I merge from master to the topic branch, which changes code
I'm actively working in and requires semantic changes (functions
renamed, moved, etc).
Once I'm ready to merge the topic branch back into master, I have two
options (bearing in mind the goal is to keep history as clean as
possible. Furthermore this implies that the constant merging into
topic from master has made the topic branch look unwieldy and
difficult to audit):
1. Do a squash merge, which keeps history clean but we lose context
for the important bits (the commits representing units of work that
contribute to the topic itself).
2. Do a final rebase prior to merging.
#2 doesn't seem to be possible due to patch ordering. For example, if
I have real commits after merge commits that depend on those changes
from master being present as a base at that point in time, the rebase
will cause the patch before it to no longer include those changes from
master.
Is there a mechanism to rebase in this situation to both achieve a
clean, linear history for the topic branch and allow fast forward
merging if desired, while still not causing superfluous conflicts due
to the merges being omitted during the rebase?
Thanks in advance for any advice in this scenario.
^ permalink raw reply
* Re: git branch --editdescription fatal error
From: Ralf Thielow @ 2017-01-06 19:31 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jake Lambert, git@vger.kernel.org
In-Reply-To: <CAGZ79kaz_hS9P7vqV9EhZazM_g0OUdRfMtFPJ7gFu-h-ku=NKw@mail.gmail.com>
2017-01-06 18:55 GMT+01:00 Stefan Beller <sbeller@google.com>:
> On Fri, Jan 6, 2017 at 9:25 AM, Jake Lambert <jake@onahill.co> wrote:
>> Hello,
>>
>> When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place.
>
> That seems strange. Is it possible that your config is not writable?
> (.git/config, ~/gitconfig, you'd need to find out where the <branch>
> is configured already via git config --global/--system/--local --list)
>
Have you actually tried to reproduce this issue? I'm on current next
and can reproduce the problem.
It seems Git is not happy with unsetting non-existent configs. And at
least the following piece of code
seems to be there since 10bea152a (Add functions git_config_set() and
git_config_set_multivar(), 2005-11-17).
config.c:2212
/* if nothing to unset, or too many matches, error out */
if ((store.seen == 0 && value == NULL) ||
I wonder why it's an error to remove something that doesn't exist
except the case we 'know' it
exists because of further actions. I don't think this is a thing, though.
Just for the record: `git config --unset branch.$branch.description`
does return with code "5" here as well.
I think removing things that don't exist should behave the same like
removing things that do exist, with
a success.
Ralf
^ permalink raw reply
* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Jeff King @ 2017-01-06 19:41 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <3d433abf-71a2-4702-f62b-e254520dc32c@kdbg.org>
On Fri, Jan 06, 2017 at 03:39:59PM +0100, Johannes Sixt wrote:
> > diff --git a/run-command.c b/run-command.c
> > index ca905a9e80..db47c429b7 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
> >
> > static void cleanup_children(int sig, int in_signal)
> > {
> > + struct child_to_clean *children_to_wait_for = NULL;
> > +
> > while (children_to_clean) {
> > struct child_to_clean *p = children_to_clean;
> > children_to_clean = p->next;
> > @@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
> > }
> >
> > kill(p->pid, sig);
> > + p->next = children_to_wait_for;
> > + children_to_wait_for = p;
> > + }
> > +
> > + while (children_to_wait_for) {
> > + struct child_to_clean *p = children_to_wait_for;
> > + children_to_wait_for = p->next;
> > +
> > + while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
> > + ; /* spin waiting for process exit or error */
> > +
> > if (!in_signal)
> > free(p);
> > }
> >
>
> This looks like the minimal change necessary. I wonder, though, whether the
> new local variable is really required. Wouldn't it be sufficient to walk the
> children_to_clean chain twice?
Yeah, I considered that. The fact that we disassemble the list in the
first loop has two side effects:
1. It lets us free the list as we go (for the !in_signal case).
2. If we were to get another signal, it makes us sort-of reentrant. We
will only kill and wait for each pid once.
Obviously (1) moves down to the lower loop, but I was trying to preserve
(2). I'm not sure if it is worth bothering, though. The way we pull
items off of the list is certainly not atomic (it does shorten the race
to a few instructions, though, versus potentially waiting on waitpid()
to return).
My bigger concern with the whole thing is whether we could hit some sort
of deadlock if the child doesn't die when we send it a signal. E.g.,
imagine we have a pipe open to the child and somebody sends SIGTERM to
us. We propagate SIGTERM to the child, and then waitpid() for it. The
child decides to ignore our SIGTERM for some reason and keep reading
until EOF on the pipe. It won't ever get it, and the two processes will
hang forever.
You can argue perhaps that the child is broken in that case. And I doubt
this could trigger when running a git sub-command. But we may add more
children in the future. Right now we use it for the new multi-file
clean/smudge filters. They use the hook feature to close the
descriptors, but note that that won't run in the in_signal case.
So I dunno. Maybe this waiting should be restricted only to certain
cases like executing git sub-commands.
-Peff
^ permalink raw reply
* Re: git branch --editdescription fatal error
From: Stefan Beller @ 2017-01-06 19:43 UTC (permalink / raw)
To: Ralf Thielow; +Cc: Jake Lambert, git@vger.kernel.org
In-Reply-To: <CAN0XMO+zzYOXF2gwr=0Tu-7T5BH7f_L+FhfD8RUenDeq3xFHjA@mail.gmail.com>
On Fri, Jan 6, 2017 at 11:31 AM, Ralf Thielow <ralf.thielow@gmail.com> wrote:
> 2017-01-06 18:55 GMT+01:00 Stefan Beller <sbeller@google.com>:
>> On Fri, Jan 6, 2017 at 9:25 AM, Jake Lambert <jake@onahill.co> wrote:
>>> Hello,
>>>
>>> When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place.
>>
>> That seems strange. Is it possible that your config is not writable?
>> (.git/config, ~/gitconfig, you'd need to find out where the <branch>
>> is configured already via git config --global/--system/--local --list)
>>
>
> Have you actually tried to reproduce this issue? I'm on current next
> and can reproduce the problem.
eh, I was on $random_version that I currently have installed
(with messed up submodule code, but otherwise close to master).
this gives hope that a bisect between master..next will give us a culprit?
>
> I think removing things that don't exist should behave the same like
> removing things that do exist, with
> a success.
I am not sure. Consider the tool "rm"
$ rm no_exist
rm: cannot remove ‘no_exist’: No such file or directory
$ echo $?
1
You have to use the --force.
For this specific use case I agree we should then set the force flag
to let the the removal of the config option succeed no matter if it
existed before.
Stefan
^ permalink raw reply
* Re: [PATCH v4 00/23] Delete directories left empty after ref deletion
From: Jeff King @ 2017-01-06 19:44 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, git, David Turner, Jacob Keller, Philip Oakley
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
On Fri, Jan 06, 2017 at 05:22:20PM +0100, Michael Haggerty wrote:
> This is v4 of this patch series. Thanks to Peff, Junio, Jake, and
> Philip for their feedback about v3. I believe I have addressed all of
> the comments about v1 [1], v2 [2], and v3 [3].
This version looks good to me.
-Peff
^ permalink raw reply
* Re: git branch --editdescription fatal error
From: Ralf Thielow @ 2017-01-06 20:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jake Lambert, git@vger.kernel.org
In-Reply-To: <CAGZ79kZOfHWP_pQGN1QcmR71Ft6ib0aPwNKX80YMT7KcK0_Stg@mail.gmail.com>
2017-01-06 20:43 GMT+01:00 Stefan Beller <sbeller@google.com>:
> On Fri, Jan 6, 2017 at 11:31 AM, Ralf Thielow <ralf.thielow@gmail.com> wrote:
>> 2017-01-06 18:55 GMT+01:00 Stefan Beller <sbeller@google.com>:
>>> On Fri, Jan 6, 2017 at 9:25 AM, Jake Lambert <jake@onahill.co> wrote:
>>>> Hello,
>>>>
>>>> When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place.
>>>
>>> That seems strange. Is it possible that your config is not writable?
>>> (.git/config, ~/gitconfig, you'd need to find out where the <branch>
>>> is configured already via git config --global/--system/--local --list)
>>>
>>
>> Have you actually tried to reproduce this issue? I'm on current next
>> and can reproduce the problem.
>
> eh, I was on $random_version that I currently have installed
> (with messed up submodule code, but otherwise close to master).
>
> this gives hope that a bisect between master..next will give us a culprit?
>
Hm.
I can confirm it appears on current master (e05806da9).
>>
>> I think removing things that don't exist should behave the same like
>> removing things that do exist, with
>> a success.
>
> I am not sure. Consider the tool "rm"
>
> $ rm no_exist
> rm: cannot remove ‘no_exist’: No such file or directory
> $ echo $?
> 1
>
> You have to use the --force.
>
> For this specific use case I agree we should then set the force flag
> to let the the removal of the config option succeed no matter if it
> existed before.
>
> Stefan
Can you think of a case where this flag should be set to false?
Ralf
^ permalink raw reply
* Re: git branch --editdescription fatal error
From: Jeff King @ 2017-01-06 20:10 UTC (permalink / raw)
To: Stefan Beller; +Cc: Ralf Thielow, Jake Lambert, git@vger.kernel.org
In-Reply-To: <CAGZ79kZOfHWP_pQGN1QcmR71Ft6ib0aPwNKX80YMT7KcK0_Stg@mail.gmail.com>
On Fri, Jan 06, 2017 at 11:43:52AM -0800, Stefan Beller wrote:
> >>> When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place.
> >>
> >> That seems strange. Is it possible that your config is not writable?
> >> (.git/config, ~/gitconfig, you'd need to find out where the <branch>
> >> is configured already via git config --global/--system/--local --list)
> >>
> >
> > Have you actually tried to reproduce this issue? I'm on current next
> > and can reproduce the problem.
>
> eh, I was on $random_version that I currently have installed
> (with messed up submodule code, but otherwise close to master).
Hmm. I can reproduce, but only in one situation: when the new
description is empty. In which case we try to delete the variable. In
other words:
[this breaks; the file remains empty and we try to delete the
nonexistent config]
$ EDITOR=true git branch --edit-description master
fatal: could not unset 'branch.master.description'
[this works; we actually set the variable]
$ EDITOR='echo foo >' git branch --edit-description master
$ git config branch.master.description
foo
[and now the unsetting works; note we have to truncate here, since
the file will be prepopulated with "foo" from the existing desc]
$ EDITOR='>' git branch --edit-description master
$ git config branch.master.description
[no output]
The history of this behavior is a bit funny.
In old versions of git, we would return a failing exit code of "1" from
git-branch, with no message.
Then in bd25f89014 (branch: die on config error when editing branch
description, 2016-02-22), we actually started returning "0"! This was
because the config code did not propagate errors from its helper
functions in all cases.
That was fixed by 9c14bb08a4 (git_config_set_multivar_in_file: all
non-zero returns are errors, 2016-04-09), giving the behavior we see
today.
So between v2.7.3 and v2.8.3, we did return 0, but I think that was a
bug (we also returned 0 for a lot of other bogus cases, too).
I could see either behavior as reasonable, but I think the right
solution would be for the branch code from bd25f89014 to use the
"gently" function set the variable, and then decide which cases should
be silently ignored, and which propagated as errors.
-Peff
^ permalink raw reply
* [PATCH 0/5] refactor unpack-trees
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <CAGZ79kaM=Uosm7KAvAP-8w59Tyfc7LZiV3ZOr=PZnBgMCzr2AA@mail.gmail.com>
unpack-trees is a central file needed for the understanding
of working tree manipulation. To help with the understanding
refactor the code to be more readable.
The first patch was a standalone patch 8 days ago;
now incorporated into this series as a v3,
reducing the scope of the checkout state.
The second patch removes a single continue statement;
it needed some digging to explain, but looks trivial.
The last 3 patches shorten the check_updates function by adding more
functions. If we ever want to parallelize file IO then these smaller
functions would be the scope to do it, keeping the check_updates as
a high level function guiding through the steps what is happening during
a working tree update.
Thanks,
Stefan
Stefan Beller (5):
unpack-trees: move checkout state into check_updates
unpack-trees: remove unneeded continue
unpack-trees: factor progress setup out of check_updates
unpack-trees: factor file removal out of check_updates
unpack-trees: factor working tree update out of check_updates
unpack-trees.c | 96 ++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 64 insertions(+), 32 deletions(-)
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply
* [PATCHv3 1/5] unpack-trees: move checkout state into check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
The checkout state was introduced via 16da134b1f9
(read-trees: refactor the unpack_trees() part, 2006-07-30). An attempt to
refactor the checkout state was done in b56aa5b268e (unpack-trees: pass
checkout state explicitly to check_updates(), 2016-09-13), but we can
go even further.
The `struct checkout state` is not used in unpack_trees apart from
initializing it, so move it into the function that makes use of it,
which is `check_updates`.
Reviewed-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index bc56195e27..55c75b4d6a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -237,77 +237,82 @@ static void display_error_msgs(struct unpack_trees_options *o)
}
/*
* Unlink the last component and schedule the leading directories for
* removal, such that empty directories get removed.
*/
static void unlink_entry(const struct cache_entry *ce)
{
if (!check_leading_path(ce->name, ce_namelen(ce)))
return;
if (remove_or_warn(ce->ce_mode, ce->name))
return;
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
-static int check_updates(struct unpack_trees_options *o,
- const struct checkout *state)
+static int check_updates(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
+ int i, errs = 0;
+
struct progress *progress = NULL;
struct index_state *index = &o->result;
- int i;
- int errs = 0;
+ struct checkout state = CHECKOUT_INIT;
+
+ state.force = 1;
+ state.quiet = 1;
+ state.refresh_cache = 1;
+ state.istate = index;
if (o->update && o->verbose_update) {
for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
const struct cache_entry *ce = index->cache[cnt];
if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
total++;
}
progress = start_progress_delay(_("Checking out files"),
total, 50, 1);
cnt = 0;
}
if (o->update)
- git_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);
+ git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_WT_REMOVE) {
display_progress(progress, ++cnt);
if (o->update && !o->dry_run)
unlink_entry(ce);
continue;
}
}
- remove_marked_cache_entries(&o->result);
+ remove_marked_cache_entries(index);
remove_scheduled_dirs();
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_UPDATE) {
if (ce->ce_flags & CE_WT_REMOVE)
die("BUG: both update and delete flags are set on %s",
ce->name);
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
- errs |= checkout_entry(ce, state, NULL);
+ errs |= checkout_entry(ce, &state, NULL);
}
}
}
stop_progress(&progress);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
return errs != 0;
}
static int verify_uptodate_sparse(const struct cache_entry *ce,
struct unpack_trees_options *o);
static int verify_absent_sparse(const struct cache_entry *ce,
enum unpack_trees_error_types,
struct unpack_trees_options *o);
@@ -1113,38 +1118,33 @@ static void mark_new_skip_worktree(struct exclude_list *el,
static int verify_absent(const struct cache_entry *,
enum unpack_trees_error_types,
struct unpack_trees_options *);
/*
* N-way merge "len" trees. Returns 0 on success, -1 on failure to manipulate the
* resulting index, -2 on failure to reflect the changes to the work tree.
*
* CE_ADDED, CE_UNPACKED and CE_NEW_SKIP_WORKTREE are used internally
*/
int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
{
int i, ret;
static struct cache_entry *dfc;
struct exclude_list el;
- struct checkout state = CHECKOUT_INIT;
if (len > MAX_UNPACK_TREES)
die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
- state.force = 1;
- state.quiet = 1;
- state.refresh_cache = 1;
- state.istate = &o->result;
memset(&el, 0, sizeof(el));
if (!core_apply_sparse_checkout || !o->update)
o->skip_sparse_checkout = 1;
if (!o->skip_sparse_checkout) {
char *sparse = git_pathdup("info/sparse-checkout");
if (add_excludes_from_file_to_list(sparse, "", 0, &el, 0) < 0)
o->skip_sparse_checkout = 1;
else
o->el = ⪙
free(sparse);
}
memset(&o->result, 0, sizeof(o->result));
o->result.initialized = 1;
@@ -1257,31 +1257,31 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
if (ret < 0)
goto return_failed;
/*
* Sparse checkout is meant to narrow down checkout area
* but it does not make sense to narrow down to empty working
* tree. This is usually a mistake in sparse checkout rules.
* Do not allow users to do that.
*/
if (o->result.cache_nr && empty_worktree) {
ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory");
goto done;
}
}
o->src_index = NULL;
- ret = check_updates(o, &state) ? (-2) : 0;
+ ret = check_updates(o) ? (-2) : 0;
if (o->dst_index) {
if (!ret) {
if (!o->result.cache_tree)
o->result.cache_tree = cache_tree();
if (!cache_tree_fully_valid(o->result.cache_tree))
cache_tree_update(&o->result,
WRITE_TREE_SILENT |
WRITE_TREE_REPAIR);
}
discard_index(o->dst_index);
*o->dst_index = o->result;
} else {
discard_index(&o->result);
}
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* Re: [PATCHv2] unpack-trees: move checkout state into check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, René Scharfe
In-Reply-To: <xmqq8tqv6051.fsf@gitster.mtv.corp.google.com>
On Sat, Dec 31, 2016 at 5:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> I'd add René's Reviewed-by: here.
done
>
> I think moving heavier and initialized variables earlier and more
> lightweight and ephemeral ones like "i" later does make it easier to
> follow. "errs" has the significance and the lifetime similar to
> cnt/total, and logically should be higher, though. It is not a big
> enough deal to reroll (but as your futzing of the variable definition
> order was not a big enough deal to do in this patch, either, so...).
I will send out a series, that is based on this patch shortly;
as I fuzzed again with the small variables, that series doesn't
apply on this version but the version to be sent out shortly.
>
> Queued. Thanks.
please replace with the following series.
^ permalink raw reply
* [PATCH 5/5] unpack-trees: factor working tree update out of check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 40 ++++++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index b954ec1233..b40c069b1b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -275,67 +275,79 @@ static struct progress *get_progress(struct unpack_trees_options *o)
struct index_state *index = &o->result;
if (!o->update || !o->verbose_update)
return NULL;
for (; cnt < index->cache_nr; cnt++) {
const struct cache_entry *ce = index->cache[cnt];
if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
total++;
}
return start_progress_delay(_("Checking out files"),
total, 50, 1);
}
-static int check_updates(struct unpack_trees_options *o)
+static int update_working_tree_files(struct unpack_trees_options *o,
+ struct progress *progress,
+ unsigned start_cnt)
{
- unsigned cnt = 0;
+ unsigned cnt = start_cnt;
int i, errs = 0;
- struct progress *progress = NULL;
struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
state.force = 1;
state.quiet = 1;
state.refresh_cache = 1;
state.istate = index;
- progress = get_progress(o);
-
- if (o->update)
- git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
-
- cnt = remove_workingtree_files(o, progress);
- remove_marked_cache_entries(index);
- remove_scheduled_dirs();
-
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_UPDATE) {
if (ce->ce_flags & CE_WT_REMOVE)
die("BUG: both update and delete flags are set on %s",
ce->name);
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
- if (o->update && !o->dry_run) {
+ if (o->update && !o->dry_run)
errs |= checkout_entry(ce, &state, NULL);
- }
}
}
+
+ return errs;
+}
+
+static int check_updates(struct unpack_trees_options *o)
+{
+ struct progress *progress = NULL;
+ struct index_state *index = &o->result;
+ int errs;
+ unsigned total_removed;
+
+ progress = get_progress(o);
+
+ if (o->update)
+ git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
+
+ total_removed = remove_workingtree_files(o, progress);
+ remove_marked_cache_entries(index);
+ remove_scheduled_dirs();
+ errs = update_working_tree_files(o, progress, total_removed);
+
stop_progress(&progress);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
return errs != 0;
}
static int verify_uptodate_sparse(const struct cache_entry *ce,
struct unpack_trees_options *o);
static int verify_absent_sparse(const struct cache_entry *ce,
enum unpack_trees_error_types,
struct unpack_trees_options *o);
static int apply_sparse_checkout(struct index_state *istate,
struct cache_entry *ce,
struct unpack_trees_options *o)
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* [PATCH 4/5] unpack-trees: factor file removal out of check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 971d091fd0..b954ec1233 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -237,30 +237,50 @@ static void display_error_msgs(struct unpack_trees_options *o)
}
/*
* Unlink the last component and schedule the leading directories for
* removal, such that empty directories get removed.
*/
static void unlink_entry(const struct cache_entry *ce)
{
if (!check_leading_path(ce->name, ce_namelen(ce)))
return;
if (remove_or_warn(ce->ce_mode, ce->name))
return;
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
+static int remove_workingtree_files(struct unpack_trees_options *o,
+ struct progress *progress)
+{
+ int i;
+ unsigned cnt = 0;
+ struct index_state *index = &o->result;
+
+ for (i = 0; i < index->cache_nr; i++) {
+ const struct cache_entry *ce = index->cache[i];
+
+ if (ce->ce_flags & CE_WT_REMOVE) {
+ display_progress(progress, ++cnt);
+ if (o->update && !o->dry_run)
+ unlink_entry(ce);
+ }
+ }
+
+ return cnt;
+}
+
static struct progress *get_progress(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
struct index_state *index = &o->result;
if (!o->update || !o->verbose_update)
return NULL;
for (; cnt < index->cache_nr; cnt++) {
const struct cache_entry *ce = index->cache[cnt];
if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
total++;
}
return start_progress_delay(_("Checking out files"),
@@ -273,39 +293,32 @@ static int check_updates(struct unpack_trees_options *o)
int i, errs = 0;
struct progress *progress = NULL;
struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
state.force = 1;
state.quiet = 1;
state.refresh_cache = 1;
state.istate = index;
progress = get_progress(o);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
- for (i = 0; i < index->cache_nr; i++) {
- const struct cache_entry *ce = index->cache[i];
- if (ce->ce_flags & CE_WT_REMOVE) {
- display_progress(progress, ++cnt);
- if (o->update && !o->dry_run)
- unlink_entry(ce);
- }
- }
+ cnt = remove_workingtree_files(o, progress);
remove_marked_cache_entries(index);
remove_scheduled_dirs();
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_UPDATE) {
if (ce->ce_flags & CE_WT_REMOVE)
die("BUG: both update and delete flags are set on %s",
ce->name);
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
errs |= checkout_entry(ce, &state, NULL);
}
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* [PATCH 3/5] unpack-trees: factor progress setup out of check_updates
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
This makes check_updates shorter and easier to understand.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index f16ef14294..971d091fd0 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -237,55 +237,63 @@ static void display_error_msgs(struct unpack_trees_options *o)
}
/*
* Unlink the last component and schedule the leading directories for
* removal, such that empty directories get removed.
*/
static void unlink_entry(const struct cache_entry *ce)
{
if (!check_leading_path(ce->name, ce_namelen(ce)))
return;
if (remove_or_warn(ce->ce_mode, ce->name))
return;
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
-static int check_updates(struct unpack_trees_options *o)
+static struct progress *get_progress(struct unpack_trees_options *o)
{
unsigned cnt = 0, total = 0;
+ struct index_state *index = &o->result;
+
+ if (!o->update || !o->verbose_update)
+ return NULL;
+
+ for (; cnt < index->cache_nr; cnt++) {
+ const struct cache_entry *ce = index->cache[cnt];
+ if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
+ total++;
+ }
+
+ return start_progress_delay(_("Checking out files"),
+ total, 50, 1);
+}
+
+static int check_updates(struct unpack_trees_options *o)
+{
+ unsigned cnt = 0;
int i, errs = 0;
struct progress *progress = NULL;
struct index_state *index = &o->result;
struct checkout state = CHECKOUT_INIT;
state.force = 1;
state.quiet = 1;
state.refresh_cache = 1;
state.istate = index;
- if (o->update && o->verbose_update) {
- for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
- const struct cache_entry *ce = index->cache[cnt];
- if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
- total++;
- }
-
- progress = start_progress_delay(_("Checking out files"),
- total, 50, 1);
- cnt = 0;
- }
+ progress = get_progress(o);
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_WT_REMOVE) {
display_progress(progress, ++cnt);
if (o->update && !o->dry_run)
unlink_entry(ce);
}
}
remove_marked_cache_entries(index);
remove_scheduled_dirs();
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* [PATCH 2/5] unpack-trees: remove unneeded continue
From: Stefan Beller @ 2017-01-06 21:03 UTC (permalink / raw)
To: gitster, l.s.r; +Cc: git, Stefan Beller
In-Reply-To: <20170106210330.31761-1-sbeller@google.com>
The continue is the last statement in the loop, so not needed.
This situation arose in 700e66d66 (2010-07-30, unpack-trees: let
read-tree -u remove index entries outside sparse area) when statements
after the continue were removed.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
unpack-trees.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 55c75b4d6a..f16ef14294 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -272,31 +272,30 @@ static int check_updates(struct unpack_trees_options *o)
progress = start_progress_delay(_("Checking out files"),
total, 50, 1);
cnt = 0;
}
if (o->update)
git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
for (i = 0; i < index->cache_nr; i++) {
const struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_WT_REMOVE) {
display_progress(progress, ++cnt);
if (o->update && !o->dry_run)
unlink_entry(ce);
- continue;
}
}
remove_marked_cache_entries(index);
remove_scheduled_dirs();
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_UPDATE) {
if (ce->ce_flags & CE_WT_REMOVE)
die("BUG: both update and delete flags are set on %s",
ce->name);
display_progress(progress, ++cnt);
ce->ce_flags &= ~CE_UPDATE;
if (o->update && !o->dry_run) {
--
2.11.0.31.g919a8d0.dirty
^ permalink raw reply related
* Re: Rebasing a branch with merges
From: Philip Oakley @ 2017-01-06 21:28 UTC (permalink / raw)
To: Robert Dailey, Git; +Cc: Johannes Schindelin
In-Reply-To: <CAHd499BREpaHHyN89a1HchyJiQzPpdo3NSfoLLGVONEmX1m19g@mail.gmail.com>
From: "Robert Dailey" <rcdailey.lists@gmail.com>
> Here's the scenario:
>
> I create a topic branch so one other developer and myself can work on
> a feature that takes 2 weeks to complete. During that 2 week period,
> changes are occurring on master that I need in my topic branch. Since
> I have a collaborator on the branch, I opt for merges instead of
> rebase.
>
> Each day I merge from master to the topic branch, which changes code
> I'm actively working in and requires semantic changes (functions
> renamed, moved, etc).
>
> Once I'm ready to merge the topic branch back into master, I have two
> options (bearing in mind the goal is to keep history as clean as
> possible. Furthermore this implies that the constant merging into
> topic from master has made the topic branch look unwieldy and
> difficult to audit):
a broader question zero;
0. Is the merge always clean? Do you always do a preparatory fixup! to
ensure that the merge will be clean?
Ensuring that the merge will be clean should greatly simplify your decision
about process.
>
> 1. Do a squash merge, which keeps history clean but we lose context
> for the important bits (the commits representing units of work that
> contribute to the topic itself).
>
> 2. Do a final rebase prior to merging.
>
> #2 doesn't seem to be possible due to patch ordering. For example, if
> I have real commits after merge commits that depend on those changes
> from master being present as a base at that point in time, the rebase
> will cause the patch before it to no longer include those changes from
> master.
How much of the historic fixups to cover changes on master do you want to
keep visible? i.e. how many fork-points are truly needed (a. by you, b. by
the project - personal knowledge vs corporate knowledge).?
>
> Is there a mechanism to rebase in this situation to both achieve a
> clean, linear history for the topic branch and allow fast forward
> merging if desired, while still not causing superfluous conflicts due
> to the merges being omitted during the rebase?
>
> Thanks in advance for any advice in this scenario.
>
Have you looked at @dscho's garden-shears scripts that he uses on
Git-for-Windows as he has to continuously rebase the Windows specific
patches on top of the progressing Git master? Very similar issues ;-)
https://github.com/git-for-windows/build-extra/blob/master/shears.sh
https://blogs.msdn.microsoft.com/visualstudioalm/2016/09/03/whats-new-in-git-for-windows-2-10/
(#Fun Facts)
--
Philip
^ permalink raw reply
* minor bug: git submodule update --init from a subdir shows wrong path
From: David Turner @ 2017-01-06 21:31 UTC (permalink / raw)
To: git, sbeller
[-- Attachment #1: Type: text/plain, Size: 411 bytes --]
I believe (from bisection) that this was introduced in the transition to
C at 3604242f080.
I've attached a repro (against master). At the time the bug was
introduced, the output in question went to stdout instead of stderr, so
the attached test case will not quite work on the older version. But if
you run under -v, you'll be able to see the bad ("foo/foo/sub" instead
of "foo/sub" or just "sub") output.
[-- Attachment #2: wrong-submodule-path.patch --]
[-- Type: text/x-patch, Size: 917 bytes --]
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 64f322c..e1deb17 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -140,6 +140,23 @@ test_expect_success 'submodule update --init --recursive from subdirectory' '
test_i18ncmp expect2 actual2
'
+cat <<EOF >expect2
+Submodule 'foo/sub' (/home/novalis/twosigma/git/t/trash directory.t7406-submodule-update/withsubs/../rebasing) registered for path 'foo/sub'
+EOF
+
+test_expect_success 'submodule update --init from and of subdirectory' '
+ git init withsubs &&
+ (cd withsubs &&
+ mkdir foo &&
+ git submodule add "$(pwd)/../rebasing" foo/sub &&
+ (cd foo &&
+ git submodule deinit -f sub &&
+ git submodule update --init sub 2>../../actual2
+ )
+ ) &&
+ test_i18ncmp expect2 actual2
+'
+
apos="'";
test_expect_success 'submodule update does not fetch already present commits' '
(cd submodule &&
^ permalink raw reply related
* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Johannes Sixt @ 2017-01-06 22:42 UTC (permalink / raw)
To: Jeff King; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170106194115.k5u5esv7t63mryvk@sigill.intra.peff.net>
Am 06.01.2017 um 20:41 schrieb Jeff King:
> On Fri, Jan 06, 2017 at 03:39:59PM +0100, Johannes Sixt wrote:
>
>>> diff --git a/run-command.c b/run-command.c
>>> index ca905a9e80..db47c429b7 100644
>>> --- a/run-command.c
>>> +++ b/run-command.c
>>> @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
>>>
>>> static void cleanup_children(int sig, int in_signal)
>>> {
>>> + struct child_to_clean *children_to_wait_for = NULL;
>>> +
>>> while (children_to_clean) {
>>> struct child_to_clean *p = children_to_clean;
>>> children_to_clean = p->next;
>>> @@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
>>> }
>>>
>>> kill(p->pid, sig);
>>> + p->next = children_to_wait_for;
>>> + children_to_wait_for = p;
>>> + }
>>> +
>>> + while (children_to_wait_for) {
>>> + struct child_to_clean *p = children_to_wait_for;
>>> + children_to_wait_for = p->next;
>>> +
>>> + while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
>>> + ; /* spin waiting for process exit or error */
>>> +
>>> if (!in_signal)
>>> free(p);
>>> }
>>>
>>
>> This looks like the minimal change necessary. I wonder, though, whether the
>> new local variable is really required. Wouldn't it be sufficient to walk the
>> children_to_clean chain twice?
>
> Yeah, I considered that. The fact that we disassemble the list in the
> first loop has two side effects:
>
> 1. It lets us free the list as we go (for the !in_signal case).
>
> 2. If we were to get another signal, it makes us sort-of reentrant. We
> will only kill and wait for each pid once.
>
> Obviously (1) moves down to the lower loop, but I was trying to preserve
> (2). I'm not sure if it is worth bothering, though.
Makes sense.
> The way we pull
> items off of the list is certainly not atomic (it does shorten the race
> to a few instructions, though, versus potentially waiting on waitpid()
> to return).
>
> My bigger concern with the whole thing is whether we could hit some sort
> of deadlock if the child doesn't die when we send it a signal. E.g.,
> imagine we have a pipe open to the child and somebody sends SIGTERM to
> us. We propagate SIGTERM to the child, and then waitpid() for it. The
> child decides to ignore our SIGTERM for some reason and keep reading
> until EOF on the pipe. It won't ever get it, and the two processes will
> hang forever.
>
> You can argue perhaps that the child is broken in that case. And I doubt
> this could trigger when running a git sub-command. But we may add more
> children in the future. Right now we use it for the new multi-file
> clean/smudge filters. They use the hook feature to close the
> descriptors, but note that that won't run in the in_signal case.
>
> So I dunno. Maybe this waiting should be restricted only to certain
> cases like executing git sub-commands.
If given it some thought.
In general, I think it is wrong to wait for child processes when a
signal was received. After all, it is the purpose of a (deadly) signal
to have the process go away. There may be programs that know it better,
like less, but git should not attempt to know better in general.
We do apply some special behavior for certain cases like we do for the
pager. And now the case with aliases is another special situation. The
parent git process only delegates to the child, and as such it is
reasonable that it binds its life time to the first child, which
executes the expanded alias.
-- Hannes
^ permalink raw reply
* Git filesystem case-insensitive to case-sensitive system broken
From: Steven Robertson @ 2017-01-06 21:56 UTC (permalink / raw)
To: git
Hello,
I was doing development on a linux box on AWS, when we found a code
bug that had me switching to running the code on a Mac instead. We
discovered that we had accidentally named two files the same when
looked at case-insensitively, which made git commands afterwards
display the wrong thing. It looked like this (ignoring some things
that aren't relevant):
$ git status
modified: tests/test_system/show_19_L.txt
no changes added to commit (use "git add" and/or "git commit -a")
$ git checkout tests/test_system/show_19_L.txt
$ git status
modified: tests/test_system/show_19_l.txt
no changes added to commit (use "git add" and/or "git commit -a")
$ git checkout tests/test_system/show_19_l.txt
$ git status
modified: tests/test_system/show_19_L.txt
no changes added to commit (use "git add" and/or "git commit -a")
$ diff tests/test_system/show_19_L.txt tests/test_system/show_19_l.txt
$
Those two files are different in our repo, and as such git thinks that
we modified one of them when we try and pull it down from github
again.
Thanks for looking at this!
-- Steven
^ 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