threads / discuss / 11654

What's in git.git (stable frozen)

Subject: What's in git.git (stable frozen)

## tl;dr

8 messages between Jan 17, 2008 and Jan 18, 2008.

replies: 7people: 4as markdown or json

Junio C Hamano· Jan 17, 2008, 03:01 UTC · lore

There are a good deal of bugfixes in 'master', the largest of which is Brandon Casey's fix to builtin-commit and others' misuse of lockfile API. A tentative fix for the issue was pushed out last night but the approach has known issues for our Windowsy friends, and this attempts to address them.

We've been taking pride that the tip of 'master' is always, without regression, more stable than any released version, but today's one might have uncovered glitches. Please help testing it so that we do not have to leave it broken for a long time if it indeed is.

I spent quite a lot of time trying to come up with a simple reproduction recipe for the builtin-commit breakage, but I couldn't. I also wanted to apply the patch to remove the use (but not implementation) of repo-config along with a removal notice in the release notes but haven't got around to.

Aside from obviously correct trivial fixes that may come from the list, what currently is not in 'master' but should be in the rc4 are very minimal. Brandon Casey's lockfile fix needs to be tested for some time, repo-config deprecation notice needs to be posted, and "\C-+" patch for gitk needs to be pulled from Paul. I see French translation for git-gui in Shawn's repository, but I do not know if it, along with other changes to git-gui, are meant for 1.5.4 or not. If they are, I'd like to include them all before tagging rc4.

----------------------------------------------------------------
* The 'master' branch has these since the last announcement
  in addition to the above.
Bill Lear (1):
  Correct spelling in diff.c comment
Brandon Casey (3):
  close_lock_file(): new function in the lockfile API
  Improve use of lockfile API
  refs.c: rework ref_locks by abstracting from underlying struct lock_file
Dan McGee (2):
  Remove usage of git- (dash) commands from email hook
  cvsimport: remove last use of repo-config from git standard tools
Dave Peticolas (1):
  Documentation: fix and clarify grammar in git-merge docs.
Dmitry Potapov (1):
  treat any file with NUL as binary
Jean-Luc Herren (1):
  Make default pre-commit hook less noisy
Junio C Hamano (4):
  Revert "builtin-commit.c: remove useless check added by faulty cut and
    paste"
  Fix git-rerere documentation
  Squelch bogus progress output from git-rebase--interactive
  Document lockfile API
Kristian Høgsberg (1):
  git-commit: fix double close(2) that can close a wrong file descriptor
Linus Torvalds (3):
  Make builtin-commit.c more careful about parenthood
  Make 'git fsck' complain about non-commit branches
  Be more careful about updating refs
Mark Drago (1):
  hg-to-git: improve popen calls
Miklos Vajna (2):
  Add using merge subtree How-To
  ls-remote: add -t and -h options.
Johannes Sixt· Jan 17, 2008, 12:56 UTC · re: Junio C Hamano · lore

Re: What's in git.git (stable frozen)

Junio C Hamano schrieb:
Show 11 quoted lines
> There are a good deal of bugfixes in 'master', the largest of
> which is Brandon Casey's fix to builtin-commit and others'
> misuse of lockfile API.  A tentative fix for the issue was
> pushed out last night but the approach has known issues for our
> Windowsy friends, and this attempts to address them.
> 
> We've been taking pride that the tip of 'master' is always,
> without regression, more stable than any released version, but
> today's one might have uncovered glitches.  Please help testing
> it so that we do not have to leave it broken for a long time if
> it indeed is.

Except for the NO_MMAP issue introduced by c3b0dec509f that kills git-fast-import in t9301* as discussed in a parallel thread, all tests pass on Windows (MinGW port). This is a good sign since most, if not all, issues around the lockfile API have been discovered by the test suite in the past.

-- Hannes
Brandon Casey· Jan 17, 2008, 16:58 UTC · re: Junio C Hamano · lore

[PATCH] fast-import.c: don't try to commit marks file if write failed

We also move the assignment of -1 to the lock file descriptor up, so that rollback_lock_file() can be called safely after a possible attempt to fclose(). This matches the contents of the 'if' statement just above testing success of fdopen().

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

Since we forget the lock file descriptor there is a chance that we will leave the file open if a write error occurs. We'll still delete the file. I don't think it's worth bending over backwards to make sure the file is closed on this failure.

-brandon
 fast-import.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 3609c24..4cf5092 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1545,19 +1545,27 @@ static void dump_marks(void)
 		return;
 	}
 
-	dump_marks_helper(f, 0, marks);
-	if (ferror(f) || fclose(f))
-		failure |= error("Unable to write marks file %s: %s",
-			mark_file, strerror(errno));
 	/*
-	 * Since the lock file was fdopen()'ed and then fclose()'ed above,
-	 * assign -1 to the lock file descriptor so that commit_lock_file()
+	 * Since the lock file was fdopen()'ed, it should not be close()'ed.
+	 * Assign -1 to the lock file descriptor so that commit_lock_file()
 	 * won't try to close() it.
 	 */
 	mark_lock.fd = -1;
-	if (commit_lock_file(&mark_lock))
-		failure |= error("Unable to write commit file %s: %s",
+
+	dump_marks_helper(f, 0, marks);
+	if (ferror(f) || fclose(f)) {
+		rollback_lock_file(&mark_lock);
+		failure |= error("Unable to write marks file %s: %s",
 			mark_file, strerror(errno));
+		return;
+	}
+
+	if (commit_lock_file(&mark_lock)) {
+		rollback_lock_file(&mark_lock);
+		failure |= error("Unable to commit marks file %s: %s",
+			mark_file, strerror(errno));
+		return;
+	}
 }
 
 static int read_next_command(void)

-- 
1.5.4.rc3.17.gb63a4
Junio C Hamano· Jan 17, 2008, 17:57 UTC · re: Brandon Casey · lore

Re: [PATCH] fast-import.c: don't try to commit marks file if write failed

Brandon Casey <casey@nrlssc.navy.mil> writes:
Show 7 quoted lines
> We also move the assignment of -1 to the lock file descriptor
> up, so that rollback_lock_file() can be called safely after a
> possible attempt to fclose(). This matches the contents of
> the 'if' statement just above testing success of fdopen().
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---

Shawn, could you lend an extra set of eyeballs on this one please? It looks sane to me but obviously you are more familiar with the code.

Show 53 quoted lines
> Since we forget the lock file descriptor there is a chance
> that we will leave the file open if a write error occurs. We'll
> still delete the file. I don't think it's worth bending
> over backwards to make sure the file is closed on this failure.
>
> -brandon
>
>
>  fast-import.c |   24 ++++++++++++++++--------
>  1 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index 3609c24..4cf5092 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1545,19 +1545,27 @@ static void dump_marks(void)
>  		return;
>  	}
>  
> -	dump_marks_helper(f, 0, marks);
> -	if (ferror(f) || fclose(f))
> -		failure |= error("Unable to write marks file %s: %s",
> -			mark_file, strerror(errno));
>  	/*
> -	 * Since the lock file was fdopen()'ed and then fclose()'ed above,
> -	 * assign -1 to the lock file descriptor so that commit_lock_file()
> +	 * Since the lock file was fdopen()'ed, it should not be close()'ed.
> +	 * Assign -1 to the lock file descriptor so that commit_lock_file()
>  	 * won't try to close() it.
>  	 */
>  	mark_lock.fd = -1;
> -	if (commit_lock_file(&mark_lock))
> -		failure |= error("Unable to write commit file %s: %s",
> +
> +	dump_marks_helper(f, 0, marks);
> +	if (ferror(f) || fclose(f)) {
> +		rollback_lock_file(&mark_lock);
> +		failure |= error("Unable to write marks file %s: %s",
>  			mark_file, strerror(errno));
> +		return;
> +	}
> +
> +	if (commit_lock_file(&mark_lock)) {
> +		rollback_lock_file(&mark_lock);
> +		failure |= error("Unable to commit marks file %s: %s",
> +			mark_file, strerror(errno));
> +		return;
> +	}
>  }
>  
>  static int read_next_command(void)
> -- 
> 1.5.4.rc3.17.gb63a4
Shawn O. Pearce· Jan 18, 2008, 02:09 UTC · re: Junio C Hamano · lore

Re: [PATCH] fast-import.c: don't try to commit marks file if write failed

Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
> > We also move the assignment of -1 to the lock file descriptor
> > up, so that rollback_lock_file() can be called safely after a
> > possible attempt to fclose(). This matches the contents of
> > the 'if' statement just above testing success of fdopen().
> >
> > Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> > ---
> 
> Shawn, could you lend an extra set of eyeballs on this one
> please?  It looks sane to me but obviously you are more familiar
> with the code.
I agree.
Acked-by: Shawn O. Pearce <spearce@spearce.org>
 
-- 
Shawn.
Brandon Casey· Jan 17, 2008, 17:17 UTC · re: Junio C Hamano · lore

Re: What's in git.git (stable frozen)

In lockfile.c:commit_locked_index()

You didn't include the portion of your patch that assigned NULL to alternate_index_output before attempting to close and rename.

Should that be included?
-brandon
Junio C Hamano wrote:
@@ -185,9 +198,15 @@ void set_alternate_index_output(const char *name)
 int commit_locked_index(struct lock_file *lk)
 {
 	if (alternate_index_output) {
-		int result = rename(lk->filename, alternate_index_output);
-		lk->filename[0] = 0;
-		return result;
+		const char *newname = alternate_index_output;
+		alternate_index_output = NULL;
Junio C Hamano· Jan 17, 2008, 17:57 UTC · re: Brandon Casey · lore

Re: What's in git.git (stable frozen)

Brandon Casey <casey@nrlssc.navy.mil> writes:
Show 7 quoted lines
> In lockfile.c:commit_locked_index()
>
> You didn't include the portion of your patch that
> assigned NULL to alternate_index_output before
> attempting to close and rename.
>
> Should that be included?

You are right, but that is a separate topic and should be a separate patch, so I dropped the hunk from that commit.

Shawn O. Pearce· Jan 18, 2008, 03:01 UTC · re: Junio C Hamano · lore

Re: What's in git.git (stable frozen)

Junio C Hamano <gitster@pobox.com> wrote:
> I see French translation for git-gui in Shawn's repository, but
> I do not know if it, along with other changes to git-gui, are
> meant for 1.5.4 or not.  If they are, I'd like to include them
> all before tagging rc4.

I have a few changes queued for 0.9.2, which I'd like to get done and included before 1.5.4, plus I'd like to get Mark Levedahl's DESTDIR issue resolved before then too.

.... changes since 0.9.1 ....
Christian Couder (2):
      git-gui: Initial french translation
      git-gui: add french glossary: glossary/fr.po
Christian Stimming (4):
      git-gui: Update glossary: add term "hunk"
      git-gui: Update German translation
      git-gui: Fix broken revert confirmation.
      git-gui: Improve German translation.
Peter Karlsson (1):
      git-gui: Updated Swedish translation after mailing list review.
Shawn O. Pearce (2):
      git-gui: Allow 'Create New Repository' on existing directories
      git-gui: Refresh file status description after hunk application
-- 
Shawn.

← back to recent threads