* Re: [PATCH] parse_object: clear "parsed" when freeing buffers
From: Jonathon Mah @ 2013-01-23 23:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vsj5rfspy.fsf@alter.siamese.dyndns.org>
[Adding Jeff King to CC; I meant to copy you in the original but forgot, sorry]
On 2013-01-23, at 14:19, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathon Mah <jmah@me.com> writes:
>
>> Add a new function "free_object_buffer", which marks the object as
>> un-parsed and frees the buffer. Only trees and commits have buffers;
>> other types are not affected. If the tree or commit buffer is already
>> NULL, the "parsed" flag is still cleared so callers can control the free
>> themselves (index-pack.c uses this).
>>
>> Several areas of code would free buffers for object structs that
>> contained them ("struct tree" and "struct commit"), but without clearing
>> the "parsed" flag. parse_object would clear the flag for "struct tree",
>> but commits would remain in an invalid state (marked as parsed but with
>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
>> invalid objects stay around and can lead to bad behavior.
>>
>> In particular, running pickaxe on all refs in a repository with a cached
>> textconv could segfault. If the textconv cache ref was evaluated first
>> by cmd_log_walk, a subsequent notes_cache_match_validity call would
>> dereference NULL.
>
> Conceptually this is a right thing to do, but it is unclear why this
> conversion is safe in the existing code.
>
> A codepath that used to free() and assign NULL to a buffer without
> resetting .parsed would have assumed that it can find out the parsed
> properties of the object (e.g. .parents) without re-parsing the
> object, and much more importantly, the modifications made by that
> codepath will not be clobbered by later call to parse_object().
>
> IIRC, revision traversal machinery rewrites commit->parents but
> discards buffer when it knows that the log message is not needed
> (save_commit_buffer controls this behaviour). I do not offhand know
> if there are other instances of existing code that depend on the
> current behaviour, but have you audited all the codepaths that are
> affected by this patch and codepaths that work on objects this patch
> unmarks their .parsed field will not have such a problem?
No, I haven't audited the code paths (I have only the loosest familiarity with the source). Indeed, I found that clearing the 'parsed' flag in fsck.c (traverse_one_object()) is incorrect and causes test failures.
With the object cache, isn't modifying the object unsafe in general? Instead of auditing code paths, it's now necessary to audit _all_ code that uses "struct object", which seems infeasible.
Anyway, I don't care about the implementation (Junio does that extremely well), I'm just trying to fix the segfault demonstrated in the test attached to the patch.
Jonathon Mah
me@JonathonMah.com
^ permalink raw reply
* Re: Re: Bug in EOL conversion?
From: Philip Oakley @ 2013-01-23 23:32 UTC (permalink / raw)
To: kusmabite; +Cc: Stefan Norgren, git, Git MsysGit
In-Reply-To: <CABPQNSaqFjvW6Kudc2uN3YWvrZuimN7MDWUeyjyG9vSZHD=C8g@mail.gmail.com>
From: "Erik Faye-Lund" <kusmabite@gmail.com>
Sent: Wednesday, January 23, 2013 10:36 PM
> On Wed, Jan 23, 2013 at 10:55 PM, Philip Oakley <philipoakley@iee.org>
> wrote:
>> The msysgit list msysgit@googlegroups.com may be a better place for
>> this.
>>
>> It is likely that you have a windows specific EOL conversion set
>> within the
>> wider config's (i.e. --system, --global). You may have core.safecrlf
>> set
>> which does a round trip test so tests the conversion both ways.
>
> The default for core.safecrlf is "warn", so one does not need a
> setting to get that warning.
>
Thank you confirming the Git for Windows default, which I don't believe
Stefan had realised was active.
I had responded to Stefan's original 'bug' report as no one had picked
up on it, and suspected it (core.safecrlf ) was set in Git for Windows,
though wasn't able to immediately check it myself.
I did not think it was a bug at all, merely a misunderstanding by Stefan
about the safety features within Git (for Windows).
--
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
^ permalink raw reply
* Re: Bug in EOL conversion?
From: Pēteris Kļaviņš @ 2013-01-23 23:04 UTC (permalink / raw)
To: msysgit; +Cc: Stefan Norgren, git, Philip Oakley
In-Reply-To: <063ABD39C46D492391698E400A7D1FA9@PhilipOakley>
[-- Attachment #1: Type: text/plain, Size: 8101 bytes --]
Hi Philip
You are mis-reading the warning. Git is acting as it does for the
recommended settings for Windows. The 'repository' under the .git directory
ALWAYS contains files with LF-only endings (unless you desperately override
the settings). The warning was telling you that when you were checking in
the file, it found that you have 'abnormal' line endings (for Windows) in
the file in your checkout (just LF endings) and so tried to be helpful in
saying that if someone else were to check the file out from your repository
on Windows, they would get CRLF in the file on checkout. But the warning
says 'the file will be checked in with CRLF' or similar because you are not
supposed to know that the repository actually strips the CR and stores the
file with LF-only. As you found out when you deleted the file and checked
out a fresh copy, Git DID give you a copy of the file with CRLF endings, as
how it said it had stored it in the repository! This translation between
CRLF on disk and LF-only in the repository is done so that the same set of
files would have the same repository contents, regardless of whether they
were originally created on Windows or Linux. On Windows, files are normally
created WITH CRLF endings, and on Linux with LF-only endings. In both
cases, the same otherwise identical files would have identical, LF-only,
copies in the actual repository in the .git directory.
Peter
On Wednesday, 23 January 2013 21:55:13 UTC, Philip Oakley wrote:
>
> The msysgit list msy...@googlegroups.com <javascript:> may be a better
> place for
> this.
>
> It is likely that you have a windows specific EOL conversion set within
> the wider config's (i.e. --system, --global). You may have
> core.safecrlf set which does a round trip test so tests the conversion
> both ways.
>
> The normal canonical line ending choice is LF in the repo.
>
> I don't have a W7 install to compare against.
>
> Philip
>
> ----- Original Message -----
> From: "Stefan Norgren" <stefan....@gmail.com <javascript:>>
> To: <g...@vger.kernel.org <javascript:>>
> Sent: Wednesday, January 23, 2013 2:44 AM
> Subject: Bug in EOL conversion?
>
>
> > Hi,
> >
> > The EOL conversion does not behave as indicated by output message from
> > add and commit. Here is my test case executed on Windows 7 64 bit.
> >
> >
> > $ git --version
> > git version 1.8.0.msysgit.0
> > $ which git
> > /cygdrive/c/Program Files (x86)/Git/cmd/git
> > $ git config --list
> > core.symlinks=false
> > core.autocrlf=true
> > color.diff=auto
> > color.status=auto
> > color.branch=auto
> > color.interactive=true
> > pack.packsizelimit=2g
> > help.format=html
> > http.sslcainfo=/bin/curl-ca-bundle.crt
> > sendemail.smtpserver=/bin/msmtp.exe
> > diff.astextplain.textconv=astextplain
> > rebase.autosquash=true
> > user.name=Stefan
> > user.email=stefan@---.com
> > core.repositoryformatversion=0
> > core.filemode=false
> > core.bare=false
> > core.logallrefupdates=true
> > core.symlinks=false
> > core.ignorecase=true
> > core.hidedotfiles=dotGitOnly
> >
> > -- Note core.autocrlf=true.
> > -- Created withcrlf.txt with one character and one CRLF line feed.
> > File size 3 bytes.
> > -- Created withlf.txt with one character and one LF line feed. File
> > size 2 bytes.
> > -- Now let's init repository.
> >
> > $ git init
> > Initialized empty Git repository in D:/Dev/workspaces/gittest/.git/
> > $ ls -la
> > total 10
> > d---------+ 1 Stefan None 0 Jan 23 02:12 .
> > d---------+ 1 Stefan None 0 Jan 23 02:10 ..
> > d---------+ 1 Stefan None 0 Jan 23 02:13 .git
> > ----------+ 1 Stefan None 3 Jan 23 01:55 withcrlf.txt
> > ----------+ 1 Stefan None 2 Jan 23 01:55 withlf.txt
> >
> > -- Note no .gitattributes file that will affect EOL conversion.
> >
> > $ ls -la .git/info/
> > total 5
> > d---------+ 1 Stefan None 0 Jan 23 02:12 .
> > d---------+ 1 Stefan None 0 Jan 23 02:13 ..
> > ----------+ 1 Stefan None 240 Jan 23 02:12 exclude
> >
> > -- Note no attribute file in .git/info/ that will affect EOL
> > conversion.
> >
> > $ git add *
> > warning: LF will be replaced by CRLF in withlf.txt.
> > The file will have its original line endings in your working
> > directory.
> > $ git commit -m 'Testing EOL'
> > [master (root-commit) 9a0b2f5] Testing EOL
> > 2 files changed, 2 insertions(+)
> > create mode 100644 withcrlf.txt
> > create mode 100644 withlf.txt
> > warning: LF will be replaced by CRLF in withlf.txt.
> > The file will have its original line endings in your working
> > directory.
> > $ ls -la
> > total 10
> > d---------+ 1 Stefan None 0 Jan 23 02:12 .
> > d---------+ 1 Stefan None 0 Jan 23 02:10 ..
> > d---------+ 1 Stefan None 0 Jan 23 02:22 .git
> > ----------+ 1 Stefan None 3 Jan 23 01:55 withcrlf.txt
> > ----------+ 1 Stefan None 2 Jan 23 01:55 withlf.txt
> >
> > -- So no changes (see file size) to files in working directory. This
> > is expected and according to output warning from add and commit.
> >
> > -- Now lets look in the repository
> >
> > $ git ls-tree -l HEAD withcrlf.txt
> > 100644 blob d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 2
> > withcrlf.txt
> > $ git ls-tree -l HEAD withlf.txt
> > 100644 blob d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 2
> > withlf.txt
> >
> > -- Note that size of withlf.txt is 2 in repository indicating that LF
> > was not replaced by CRLF in withlf.txt as indicated in output from add
> > and commit. Also note that size of withcrlf.txt is also 2 in
> > repository so it looks like CRLF was replaced by LF in withcrlf.txt.
> > To verify I will delete the files from working directory, turn off EOL
> > conversion, checkout files and look at file endings in the working
> > directory.
> >
> > $ rm with*
> > $ ls -la
> > total 8
> > d---------+ 1 Stefan None 0 Jan 23 02:31 .
> > d---------+ 1 Stefan None 0 Jan 23 02:10 ..
> > d---------+ 1 Stefan None 0 Jan 23 02:22 .git
> > $ git status
> > # On branch master
> > # Changes not staged for commit:
> > # (use "git add/rm <file>..." to update what will be committed)
> > # (use "git checkout -- <file>..." to discard changes in working
> > directory)
> > #
> > # deleted: withcrlf.txt
> > # deleted: withlf.txt
> > #
> > no changes added to commit (use "git add" and/or "git commit -a")
> > $ git config --global core.autocrlf false
> > $ git config --global core.autocrlf
> > false
> > $ git checkout -- with*
> > $ ls -la
> > total 10
> > d---------+ 1 Stefan None 0 Jan 23 02:38 .
> > d---------+ 1 Stefan None 0 Jan 23 02:10 ..
> > d---------+ 1 Stefan None 0 Jan 23 02:38 .git
> > ----------+ 1 Stefan None 2 Jan 23 02:38 withcrlf.txt
> > ----------+ 1 Stefan None 2 Jan 23 02:38 withlf.txt
> >
> > -- Both files in working directory files now have LF line endings
> > confirming that files in repository have LF file endings. Either the
> > output message of add and commit is wrong or the behavior of the EOL
> > conversion is wrong... or... have I missed something...?
> >
> > /Stefan
> > --
> > To unsubscribe from this list: send the line "unsubscribe git" in
> > the body of a message to majo...@vger.kernel.org <javascript:>
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> >
> > -----
> > No virus found in this message.
> > Checked by AVG - www.avg.com
> > Version: 2013.0.2890 / Virus Database: 2639/6050 - Release Date:
> > 01/22/13
> >
>
>
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
[-- Attachment #2: Type: text/html, Size: 9918 bytes --]
^ permalink raw reply
* Re: Bug in EOL conversion?
From: Thomas Rast @ 2013-01-23 22:46 UTC (permalink / raw)
To: Stefan Norgren; +Cc: git
In-Reply-To: <CANrZfmGXtKcB+i_xhNJELftRc1pC2TJKKhOieHm=5Qkni9OKrA@mail.gmail.com>
Stefan Norgren <stefan.norgren@gmail.com> writes:
> $ git add *
> warning: LF will be replaced by CRLF in withlf.txt.
> The file will have its original line endings in your working directory.
[...]
> $ ls -la
> total 10
> d---------+ 1 Stefan None 0 Jan 23 02:12 .
> d---------+ 1 Stefan None 0 Jan 23 02:10 ..
> d---------+ 1 Stefan None 0 Jan 23 02:22 .git
> ----------+ 1 Stefan None 3 Jan 23 01:55 withcrlf.txt
> ----------+ 1 Stefan None 2 Jan 23 01:55 withlf.txt
[...]
> $ git ls-tree -l HEAD withcrlf.txt
> 100644 blob d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 2 withcrlf.txt
> $ git ls-tree -l HEAD withlf.txt
> 100644 blob d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 2 withlf.txt
Isn't that what would be expected? It's a combination of
- the canonical representation of a newline is LF, so the repository
stores LF
- with safecrlf, checkout converts LF->CRLF and add converts CRLF->LF
So from the user's POV, running
git add withlf.txt
rm withlf.txt
git checkout -- withlf.txt
would appear to replace LF with CRLF in the worktree. That's what the
message says.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: Re: Bug in EOL conversion?
From: Erik Faye-Lund @ 2013-01-23 22:36 UTC (permalink / raw)
To: Philip Oakley; +Cc: Stefan Norgren, git, Git MsysGit
In-Reply-To: <063ABD39C46D492391698E400A7D1FA9@PhilipOakley>
On Wed, Jan 23, 2013 at 10:55 PM, Philip Oakley <philipoakley@iee.org> wrote:
> The msysgit list msysgit@googlegroups.com may be a better place for this.
>
> It is likely that you have a windows specific EOL conversion set within the
> wider config's (i.e. --system, --global). You may have core.safecrlf set
> which does a round trip test so tests the conversion both ways.
The default for core.safecrlf is "warn", so one does not need a
setting to get that warning.
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
^ permalink raw reply
* Re: [PATCH] parse_object: clear "parsed" when freeing buffers
From: Junio C Hamano @ 2013-01-23 22:19 UTC (permalink / raw)
To: Jonathon Mah; +Cc: git
In-Reply-To: <8988071A-1DF3-463E-8AF9-AE4EA200D786@me.com>
Jonathon Mah <jmah@me.com> writes:
> Add a new function "free_object_buffer", which marks the object as
> un-parsed and frees the buffer. Only trees and commits have buffers;
> other types are not affected. If the tree or commit buffer is already
> NULL, the "parsed" flag is still cleared so callers can control the free
> themselves (index-pack.c uses this).
>
> Several areas of code would free buffers for object structs that
> contained them ("struct tree" and "struct commit"), but without clearing
> the "parsed" flag. parse_object would clear the flag for "struct tree",
> but commits would remain in an invalid state (marked as parsed but with
> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
> invalid objects stay around and can lead to bad behavior.
>
> In particular, running pickaxe on all refs in a repository with a cached
> textconv could segfault. If the textconv cache ref was evaluated first
> by cmd_log_walk, a subsequent notes_cache_match_validity call would
> dereference NULL.
Conceptually this is a right thing to do, but it is unclear why this
conversion is safe in the existing code.
A codepath that used to free() and assign NULL to a buffer without
resetting .parsed would have assumed that it can find out the parsed
properties of the object (e.g. .parents) without re-parsing the
object, and much more importantly, the modifications made by that
codepath will not be clobbered by later call to parse_object().
IIRC, revision traversal machinery rewrites commit->parents but
discards buffer when it knows that the log message is not needed
(save_commit_buffer controls this behaviour). I do not offhand know
if there are other instances of existing code that depend on the
current behaviour, but have you audited all the codepaths that are
affected by this patch and codepaths that work on objects this patch
unmarks their .parsed field will not have such a problem?
^ permalink raw reply
* [PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
From: Junio C Hamano @ 2013-01-23 21:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358978130-12216-1-git-send-email-gitster@pobox.com>
When we push to update an existing ref, if:
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
it won't be correct to suggest to fetch, integrate and push again,
as the old and new objects will not "merge".
If we do not have the current object at the tip of the remote, we do
not even know that object, when fetched, is something that can be
merged. In such a case, suggesting to pull first just like
non-fast-forward case may not be technically correct, but in
practice, most such failures are seen when you try to push your work
to a branch without knowing that somebody else already pushed to
update the same branch since you forked, so "pull first" would work
as a suggestion most of the time.
In these cases, the current code already rejects such a push on the
client end, but we used the same error and advice messages as the
ones used when rejecting a non-fast-forward push, i.e. pull from
there and integrate before pushing again. Introduce new
rejection reasons and reword the messages appropriately.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/config.txt | 12 +++++++++++-
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 29 +++++++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 2 ++
remote.c | 11 ++++++++---
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 12 ++++++++++++
transport.h | 2 ++
11 files changed, 92 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 90e7d10..1f47761 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -143,7 +143,8 @@ advice.*::
pushUpdateRejected::
Set this variable to 'false' if you want to disable
'pushNonFFCurrent', 'pushNonFFDefault',
- 'pushNonFFMatching', and 'pushAlreadyExists'
+ 'pushNonFFMatching', 'pushAlreadyExists',
+ 'pushFetchFirst', and 'pushNeedsForce'
simultaneously.
pushNonFFCurrent::
Advice shown when linkgit:git-push[1] fails due to a
@@ -162,6 +163,15 @@ advice.*::
pushAlreadyExists::
Shown when linkgit:git-push[1] rejects an update that
does not qualify for fast-forwarding (e.g., a tag.)
+ pushFetchFirst::
+ Shown when linkgit:git-push[1] rejects an update that
+ tries to overwrite a remote ref that points at an
+ object we do not have.
+ pushNeedsForce::
+ Shown when linkgit:git-push[1] rejects an update that
+ tries to overwrite a remote ref that points at an
+ object that is not a committish, or make the remote
+ ref point at an object that is not a committish.
statusHints::
Show directions on how to proceed from the current
state in the output of linkgit:git-status[1] and in
diff --git a/advice.c b/advice.c
index d287927..780f58d 100644
--- a/advice.c
+++ b/advice.c
@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;
int advice_push_non_ff_default = 1;
int advice_push_non_ff_matching = 1;
int advice_push_already_exists = 1;
+int advice_push_fetch_first = 1;
+int advice_push_needs_force = 1;
int advice_status_hints = 1;
int advice_commit_before_merge = 1;
int advice_resolve_conflict = 1;
@@ -20,6 +22,8 @@ static struct {
{ "pushnonffdefault", &advice_push_non_ff_default },
{ "pushnonffmatching", &advice_push_non_ff_matching },
{ "pushalreadyexists", &advice_push_already_exists },
+ { "pushfetchfirst", &advice_push_fetch_first },
+ { "pushneedsforce", &advice_push_needs_force },
{ "statushints", &advice_status_hints },
{ "commitbeforemerge", &advice_commit_before_merge },
{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 8bf6356..fad36df 100644
--- a/advice.h
+++ b/advice.h
@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;
extern int advice_push_non_ff_default;
extern int advice_push_non_ff_matching;
extern int advice_push_already_exists;
+extern int advice_push_fetch_first;
+extern int advice_push_needs_force;
extern int advice_status_hints;
extern int advice_commit_before_merge;
extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 8491e43..92ca3d7 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -220,10 +220,21 @@ static const char message_advice_checkout_pull_push[] =
"(e.g. 'git pull') before pushing again.\n"
"See the 'Note about fast-forwards' in 'git push --help' for details.");
+static const char message_advice_ref_fetch_first[] =
+ N_("Updates were rejected because you do not have the object at the tip\n"
+ "of the remote. You may want to first merge the remote changes (e.g.\n"
+ " 'git pull') before pushing again.\n"
+ "See the 'Note about fast-forwards' in 'git push --help' for details.");
+
static const char message_advice_ref_already_exists[] =
N_("Updates were rejected because the destination reference already exists\n"
"in the remote.");
+static const char message_advice_ref_needs_force[] =
+ N_("You cannot update a remote ref that points at a non-commit object,\n"
+ "or update a remote ref to make it point at a non-commit object,\n"
+ "without using the '--force' option.\n");
+
static void advise_pull_before_push(void)
{
if (!advice_push_non_ff_current || !advice_push_update_rejected)
@@ -252,6 +263,20 @@ static void advise_ref_already_exists(void)
advise(_(message_advice_ref_already_exists));
}
+static void advise_ref_fetch_first(void)
+{
+ if (!advice_push_fetch_first || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_fetch_first));
+}
+
+static void advise_ref_needs_force(void)
+{
+ if (!advice_push_needs_force || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_needs_force));
+}
+
static int push_with_options(struct transport *transport, int flags)
{
int err;
@@ -285,6 +310,10 @@ static int push_with_options(struct transport *transport, int flags)
advise_checkout_pull_push();
} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
advise_ref_already_exists();
+ } else if (reject_reasons & REJECT_FETCH_FIRST) {
+ advise_ref_fetch_first();
+ } else if (reject_reasons & REJECT_NEEDS_FORCE) {
+ advise_ref_needs_force();
}
return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f849e0a..57a46b2 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,16 @@ static void print_helper_status(struct ref *ref)
msg = "non-fast forward";
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ res = "error";
+ msg = "fetch first";
+ break;
+
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ res = "error";
+ msg = "needs force";
+ break;
+
case REF_STATUS_REJECT_ALREADY_EXISTS:
res = "error";
msg = "already exists";
diff --git a/cache.h b/cache.h
index 12631a1..377a3df 100644
--- a/cache.h
+++ b/cache.h
@@ -1010,6 +1010,8 @@ struct ref {
REF_STATUS_REJECT_NONFASTFORWARD,
REF_STATUS_REJECT_ALREADY_EXISTS,
REF_STATUS_REJECT_NODELETE,
+ REF_STATUS_REJECT_FETCH_FIRST,
+ REF_STATUS_REJECT_NEEDS_FORCE,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
REF_STATUS_EXPECTING_REPORT
diff --git a/remote.c b/remote.c
index 969aa11..a772e74 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,8 +1322,12 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
if (!prefixcmp(ref->name, "refs/tags/"))
why = REF_STATUS_REJECT_ALREADY_EXISTS;
- else if (!has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1))
+ else if (!has_sha1_file(ref->old_sha1))
+ why = REF_STATUS_REJECT_FETCH_FIRST;
+ else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
+ !lookup_commit_reference_gently(ref->new_sha1, 1))
+ why = REF_STATUS_REJECT_NEEDS_FORCE;
+ else if (!ref_newer(ref->new_sha1, ref->old_sha1))
why = REF_STATUS_REJECT_NONFASTFORWARD;
if (!force_ref_update)
@@ -1512,7 +1516,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
struct commit_list *list, *used;
int found = 0;
- /* Both new and old must be commit-ish and new is descendant of
+ /*
+ * Both new and old must be commit-ish and new is descendant of
* old. Otherwise we require --force.
*/
o = deref_tag(parse_object(old_sha1), NULL, 0);
diff --git a/send-pack.c b/send-pack.c
index 1c375f0..97ab336 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,
switch (ref->status) {
case REF_STATUS_REJECT_NONFASTFORWARD:
case REF_STATUS_REJECT_ALREADY_EXISTS:
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ case REF_STATUS_REJECT_NEEDS_FORCE:
case REF_STATUS_UPTODATE:
continue;
default:
diff --git a/transport-helper.c b/transport-helper.c
index 965b778..cb3ef7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -666,6 +666,16 @@ static void push_update_ref_status(struct strbuf *buf,
free(msg);
msg = NULL;
}
+ else if (!strcmp(msg, "fetch first")) {
+ status = REF_STATUS_REJECT_FETCH_FIRST;
+ free(msg);
+ msg = NULL;
+ }
+ else if (!strcmp(msg, "needs force")) {
+ status = REF_STATUS_REJECT_NEEDS_FORCE;
+ free(msg);
+ msg = NULL;
+ }
}
if (*ref)
diff --git a/transport.c b/transport.c
index 585ebcd..5105562 100644
--- a/transport.c
+++ b/transport.c
@@ -699,6 +699,14 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
"already exists", porcelain);
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "fetch first", porcelain);
+ break;
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "needs force", porcelain);
+ break;
case REF_STATUS_REMOTE_REJECT:
print_ref_status('!', "[remote rejected]", ref,
ref->deletion ? NULL : ref->peer_ref,
@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,
*reject_reasons |= REJECT_NON_FF_OTHER;
} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
*reject_reasons |= REJECT_ALREADY_EXISTS;
+ } else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {
+ *reject_reasons |= REJECT_FETCH_FIRST;
+ } else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {
+ *reject_reasons |= REJECT_NEEDS_FORCE;
}
}
}
diff --git a/transport.h b/transport.h
index bfd2df5..c818763 100644
--- a/transport.h
+++ b/transport.h
@@ -143,6 +143,8 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
#define REJECT_NON_FF_HEAD 0x01
#define REJECT_NON_FF_OTHER 0x02
#define REJECT_ALREADY_EXISTS 0x04
+#define REJECT_FETCH_FIRST 0x08
+#define REJECT_NEEDS_FORCE 0x10
int transport_push(struct transport *connection,
int refspec_nr, const char **refspec, int flags,
--
1.8.1.1.517.g0318d2b
^ permalink raw reply related
* [PATCH v4 2/3] push: further simplify the logic to assign rejection reason
From: Junio C Hamano @ 2013-01-23 21:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358978130-12216-1-git-send-email-gitster@pobox.com>
First compute the reason why this push would fail if done without
"--force", and then fail it by assigning that reason when the push
was not forced (or if there is no reason to require force, allow it
to succeed).
Record the fact that the push was forced in the forced_update field
only when the push would have failed without the option.
The code becomes shorter, less repetitive and easier to read this
way, especially given that the set of rejection reasons will be
extended in a later patch.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
remote.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/remote.c b/remote.c
index 3375914..969aa11 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,23 +1318,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
*/
if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
- int nonfastforward =
- !has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1);
-
- if (!prefixcmp(ref->name, "refs/tags/")) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
- continue;
- }
- ref->forced_update = 1;
- } else if (nonfastforward) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
- continue;
- }
+ int why = 0; /* why would this push require --force? */
+
+ if (!prefixcmp(ref->name, "refs/tags/"))
+ why = REF_STATUS_REJECT_ALREADY_EXISTS;
+ else if (!has_sha1_file(ref->old_sha1)
+ || !ref_newer(ref->new_sha1, ref->old_sha1))
+ why = REF_STATUS_REJECT_NONFASTFORWARD;
+
+ if (!force_ref_update)
+ ref->status = why;
+ else if (why)
ref->forced_update = 1;
- }
}
}
}
--
1.8.1.1.517.g0318d2b
^ permalink raw reply related
* [PATCH v4 1/3] push: further clean up fields of "struct ref"
From: Junio C Hamano @ 2013-01-23 21:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358978130-12216-1-git-send-email-gitster@pobox.com>
The "nonfastforward" and "update" fields are only used while
deciding what value to assign to the "status" locally in a single
function. Remove them from the "struct ref".
The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere. It is used by status reporting code that
the particular update was "forced". Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 4 +---
remote.c | 16 ++++++----------
transport.c | 2 +-
3 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/cache.h b/cache.h
index a942bbd..12631a1 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,10 +1001,8 @@ struct ref {
char *symref;
unsigned int
force:1,
- requires_force:1,
+ forced_update:1,
merge:1,
- nonfastforward:1,
- update:1,
deletion:1;
enum {
REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index d3a1ca2..3375914 100644
--- a/remote.c
+++ b/remote.c
@@ -1317,27 +1317,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
* passing the --force argument
*/
- ref->update =
- !ref->deletion &&
- !is_null_sha1(ref->old_sha1);
-
- if (ref->update) {
- ref->nonfastforward =
+ if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
+ int nonfastforward =
!has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1);
+ || !ref_newer(ref->new_sha1, ref->old_sha1);
if (!prefixcmp(ref->name, "refs/tags/")) {
- ref->requires_force = 1;
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
continue;
}
- } else if (ref->nonfastforward) {
- ref->requires_force = 1;
+ ref->forced_update = 1;
+ } else if (nonfastforward) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
continue;
}
+ ref->forced_update = 1;
}
}
}
diff --git a/transport.c b/transport.c
index 2673d27..585ebcd 100644
--- a/transport.c
+++ b/transport.c
@@ -659,7 +659,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
const char *msg;
strcpy(quickref, status_abbrev(ref->old_sha1));
- if (ref->requires_force) {
+ if (ref->forced_update) {
strcat(quickref, "...");
type = '+';
msg = "forced update";
--
1.8.1.1.517.g0318d2b
^ permalink raw reply related
* [PATCH v4 0/3] Finishing touches to "push" advises
From: Junio C Hamano @ 2013-01-23 21:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <20130121234002.GE17156@sigill.intra.peff.net>
This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).
The series applies on top of 256b9d7 (push: fix "refs/tags/
hierarchy cannot be updated without --force", 2013-01-16).
This fourth round swaps the order of clean-up patches and now the
bottom two are clean-up patches. The main change is in the third
one.
When the object at the tip of the remote is not a committish, or the
object we are pushing is not a committish, the existing code already
rejects such a push on the client end, but we used the same error
and advice messages as the ones used when rejecting a push that does
not fast-forward, i.e. pull from there and integrate before pushing
again. Introduce a new rejection reason NEEDS_FORCE and explain why
the push was rejected, stressing the fact that --force is required
when non committish objects are involved, so that the user can (1)
notice a possibly mistyped source object name or destination ref
name, when the user is trying to push an ordinary commit, or (2)
learn that "--force" is an appropriate thing to use when the user is
sure that s/he wants to push a non-committish (which is unusual).
Unlike the third round, we do not say "fetch first, inspect the
situation to decide what to do", when we do not have the object
sitting at the tip of the remote. Most likely, it is a commit
somebody who has been working on the same branch pushed that we
haven't fetched yet, so suggesting to pull is often sufficient and
appropriate, and in a more uncommon case in which the unknown object
is not a committish, the suggested pull will fail without making
permanent damage anywhere. Next atttempt to push without changing
anything (e.g. "reset --hard") will then trigger the NEEDS_FORCE
"Your push involves non-commit objects" case.
Junio C Hamano (3):
push: further clean up fields of "struct ref"
push: further simplify the logic to assign rejection reason
push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
Documentation/config.txt | 12 +++++++++++-
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 29 +++++++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 6 +++---
remote.c | 42 +++++++++++++++++++-----------------------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 14 +++++++++++++-
transport.h | 2 ++
11 files changed, 105 insertions(+), 28 deletions(-)
--
1.8.1.1.517.g0318d2b
^ permalink raw reply
* Re: Bug in EOL conversion?
From: Philip Oakley @ 2013-01-23 21:55 UTC (permalink / raw)
To: Stefan Norgren, git; +Cc: Git MsysGit
In-Reply-To: <CANrZfmGXtKcB+i_xhNJELftRc1pC2TJKKhOieHm=5Qkni9OKrA@mail.gmail.com>
The msysgit list msysgit@googlegroups.com may be a better place for
this.
It is likely that you have a windows specific EOL conversion set within
the wider config's (i.e. --system, --global). You may have
core.safecrlf set which does a round trip test so tests the conversion
both ways.
The normal canonical line ending choice is LF in the repo.
I don't have a W7 install to compare against.
Philip
----- Original Message -----
From: "Stefan Norgren" <stefan.norgren@gmail.com>
To: <git@vger.kernel.org>
Sent: Wednesday, January 23, 2013 2:44 AM
Subject: Bug in EOL conversion?
> Hi,
>
> The EOL conversion does not behave as indicated by output message from
> add and commit. Here is my test case executed on Windows 7 64 bit.
>
>
> $ git --version
> git version 1.8.0.msysgit.0
> $ which git
> /cygdrive/c/Program Files (x86)/Git/cmd/git
> $ git config --list
> core.symlinks=false
> core.autocrlf=true
> color.diff=auto
> color.status=auto
> color.branch=auto
> color.interactive=true
> pack.packsizelimit=2g
> help.format=html
> http.sslcainfo=/bin/curl-ca-bundle.crt
> sendemail.smtpserver=/bin/msmtp.exe
> diff.astextplain.textconv=astextplain
> rebase.autosquash=true
> user.name=Stefan
> user.email=stefan@---.com
> core.repositoryformatversion=0
> core.filemode=false
> core.bare=false
> core.logallrefupdates=true
> core.symlinks=false
> core.ignorecase=true
> core.hidedotfiles=dotGitOnly
>
> -- Note core.autocrlf=true.
> -- Created withcrlf.txt with one character and one CRLF line feed.
> File size 3 bytes.
> -- Created withlf.txt with one character and one LF line feed. File
> size 2 bytes.
> -- Now let's init repository.
>
> $ git init
> Initialized empty Git repository in D:/Dev/workspaces/gittest/.git/
> $ ls -la
> total 10
> d---------+ 1 Stefan None 0 Jan 23 02:12 .
> d---------+ 1 Stefan None 0 Jan 23 02:10 ..
> d---------+ 1 Stefan None 0 Jan 23 02:13 .git
> ----------+ 1 Stefan None 3 Jan 23 01:55 withcrlf.txt
> ----------+ 1 Stefan None 2 Jan 23 01:55 withlf.txt
>
> -- Note no .gitattributes file that will affect EOL conversion.
>
> $ ls -la .git/info/
> total 5
> d---------+ 1 Stefan None 0 Jan 23 02:12 .
> d---------+ 1 Stefan None 0 Jan 23 02:13 ..
> ----------+ 1 Stefan None 240 Jan 23 02:12 exclude
>
> -- Note no attribute file in .git/info/ that will affect EOL
> conversion.
>
> $ git add *
> warning: LF will be replaced by CRLF in withlf.txt.
> The file will have its original line endings in your working
> directory.
> $ git commit -m 'Testing EOL'
> [master (root-commit) 9a0b2f5] Testing EOL
> 2 files changed, 2 insertions(+)
> create mode 100644 withcrlf.txt
> create mode 100644 withlf.txt
> warning: LF will be replaced by CRLF in withlf.txt.
> The file will have its original line endings in your working
> directory.
> $ ls -la
> total 10
> d---------+ 1 Stefan None 0 Jan 23 02:12 .
> d---------+ 1 Stefan None 0 Jan 23 02:10 ..
> d---------+ 1 Stefan None 0 Jan 23 02:22 .git
> ----------+ 1 Stefan None 3 Jan 23 01:55 withcrlf.txt
> ----------+ 1 Stefan None 2 Jan 23 01:55 withlf.txt
>
> -- So no changes (see file size) to files in working directory. This
> is expected and according to output warning from add and commit.
>
> -- Now lets look in the repository
>
> $ git ls-tree -l HEAD withcrlf.txt
> 100644 blob d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 2
> withcrlf.txt
> $ git ls-tree -l HEAD withlf.txt
> 100644 blob d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 2
> withlf.txt
>
> -- Note that size of withlf.txt is 2 in repository indicating that LF
> was not replaced by CRLF in withlf.txt as indicated in output from add
> and commit. Also note that size of withcrlf.txt is also 2 in
> repository so it looks like CRLF was replaced by LF in withcrlf.txt.
> To verify I will delete the files from working directory, turn off EOL
> conversion, checkout files and look at file endings in the working
> directory.
>
> $ rm with*
> $ ls -la
> total 8
> d---------+ 1 Stefan None 0 Jan 23 02:31 .
> d---------+ 1 Stefan None 0 Jan 23 02:10 ..
> d---------+ 1 Stefan None 0 Jan 23 02:22 .git
> $ git status
> # On branch master
> # Changes not staged for commit:
> # (use "git add/rm <file>..." to update what will be committed)
> # (use "git checkout -- <file>..." to discard changes in working
> directory)
> #
> # deleted: withcrlf.txt
> # deleted: withlf.txt
> #
> no changes added to commit (use "git add" and/or "git commit -a")
> $ git config --global core.autocrlf false
> $ git config --global core.autocrlf
> false
> $ git checkout -- with*
> $ ls -la
> total 10
> d---------+ 1 Stefan None 0 Jan 23 02:38 .
> d---------+ 1 Stefan None 0 Jan 23 02:10 ..
> d---------+ 1 Stefan None 0 Jan 23 02:38 .git
> ----------+ 1 Stefan None 2 Jan 23 02:38 withcrlf.txt
> ----------+ 1 Stefan None 2 Jan 23 02:38 withlf.txt
>
> -- Both files in working directory files now have LF line endings
> confirming that files in repository have LF file endings. Either the
> output message of add and commit is wrong or the behavior of the EOL
> conversion is wrong... or... have I missed something...?
>
> /Stefan
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> -----
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2013.0.2890 / Virus Database: 2639/6050 - Release Date:
> 01/22/13
>
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
^ permalink raw reply
* [PATCH] parse_object: clear "parsed" when freeing buffers
From: Jonathon Mah @ 2013-01-23 21:25 UTC (permalink / raw)
To: git
Add a new function "free_object_buffer", which marks the object as
un-parsed and frees the buffer. Only trees and commits have buffers;
other types are not affected. If the tree or commit buffer is already
NULL, the "parsed" flag is still cleared so callers can control the free
themselves (index-pack.c uses this).
Several areas of code would free buffers for object structs that
contained them ("struct tree" and "struct commit"), but without clearing
the "parsed" flag. parse_object would clear the flag for "struct tree",
but commits would remain in an invalid state (marked as parsed but with
a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
invalid objects stay around and can lead to bad behavior.
In particular, running pickaxe on all refs in a repository with a cached
textconv could segfault. If the textconv cache ref was evaluated first
by cmd_log_walk, a subsequent notes_cache_match_validity call would
dereference NULL.
Signed-off-by: Jonathon Mah <me@JonathonMah.com>
---
I found an old email where Jeff noted that this would be bad (yet the buffer manipulation remained).
<http://permalink.gmane.org/gmane.comp.version-control.git/188000>
builtin/fsck.c | 17 ++---------------
builtin/index-pack.c | 3 +++
builtin/log.c | 9 +++------
builtin/reflog.c | 3 +--
builtin/rev-list.c | 3 +--
http-push.c | 3 +--
list-objects.c | 3 +--
object.c | 25 +++++++++++++++++++++++--
object.h | 3 +++
reachable.c | 3 +--
revision.c | 3 +--
t/t4042-diff-textconv-caching.sh | 11 +++++++++++
upload-pack.c | 3 +--
walker.c | 5 +----
14 files changed, 53 insertions(+), 41 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index bb9a2cd..82b3612 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -133,10 +133,7 @@ static int traverse_one_object(struct object *obj)
return 1; /* error already displayed */
}
result = fsck_walk(obj, mark_object, obj);
- if (tree) {
- free(tree->buffer);
- tree->buffer = NULL;
- }
+ free_object_buffer(obj);
return result;
}
@@ -303,26 +300,16 @@ static int fsck_obj(struct object *obj)
if (fsck_object(obj, check_strict, fsck_error_func))
return -1;
- if (obj->type == OBJ_TREE) {
- struct tree *item = (struct tree *) obj;
-
- free(item->buffer);
- item->buffer = NULL;
- }
+ free_object_buffer(obj);
if (obj->type == OBJ_COMMIT) {
struct commit *commit = (struct commit *) obj;
-
- free(commit->buffer);
- commit->buffer = NULL;
-
if (!commit->parents && show_root)
printf("root %s\n", sha1_to_hex(commit->object.sha1));
}
if (obj->type == OBJ_TAG) {
struct tag *tag = (struct tag *) obj;
-
if (show_tags && tag->tagged) {
printf("tagged %s %s", typename(tag->tagged->type), sha1_to_hex(tag->tagged->sha1));
printf(" (%s) in %s\n", tag->tag, sha1_to_hex(tag->object.sha1));
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 43d364b..0eb39ae 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -750,13 +750,16 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
if (fsck_walk(obj, mark_link, NULL))
die(_("Not all child objects of %s are reachable"), sha1_to_hex(obj->sha1));
+ /* set buffer to NULL so it isn't freed */
if (obj->type == OBJ_TREE) {
struct tree *item = (struct tree *) obj;
item->buffer = NULL;
+ free_object_buffer(obj);
}
if (obj->type == OBJ_COMMIT) {
struct commit *commit = (struct commit *) obj;
commit->buffer = NULL;
+ free_object_buffer(obj);
}
obj->flags |= FLAG_CHECKED;
}
diff --git a/builtin/log.c b/builtin/log.c
index e7b7db1..433b874 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -305,11 +305,9 @@ static int cmd_log_walk(struct rev_info *rev)
* but we didn't actually show the commit.
*/
rev->max_count++;
- if (!rev->reflog_info) {
+ if (!rev->reflog_info)
/* we allow cycles in reflog ancestry */
- free(commit->buffer);
- commit->buffer = NULL;
- }
+ free_object_buffer(&commit->object);
free_commit_list(commit->parents);
commit->parents = NULL;
if (saved_nrl < rev->diffopt.needed_rename_limit)
@@ -1399,8 +1397,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
reopen_stdout(numbered_files ? NULL : commit, NULL, &rev, quiet))
die(_("Failed to create output files"));
shown = log_tree_commit(&rev, commit);
- free(commit->buffer);
- commit->buffer = NULL;
+ free_object_buffer(&commit->object);
/* We put one extra blank line between formatted
* patches and this flag is used by log-tree code
diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27..c9a660f 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -94,8 +94,7 @@ static int tree_is_complete(const unsigned char *sha1)
complete = 0;
}
}
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(tree->buffer);
if (complete)
tree->object.flags |= SEEN;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 67701be..6855892 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -170,8 +170,7 @@ static void finish_commit(struct commit *commit, void *data)
free_commit_list(commit->parents);
commit->parents = NULL;
}
- free(commit->buffer);
- commit->buffer = NULL;
+ free_object_buffer(&commit->object);
}
static void finish_object(struct object *obj,
diff --git a/http-push.c b/http-push.c
index 8701c12..042a410 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1347,8 +1347,7 @@ static struct object_list **process_tree(struct tree *tree,
break;
}
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(obj);
return p;
}
diff --git a/list-objects.c b/list-objects.c
index 3dd4a96..fb4b531 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -123,8 +123,7 @@ static void process_tree(struct rev_info *revs,
cb_data);
}
strbuf_setlen(base, baselen);
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(obj);
}
static void mark_edge_parents_uninteresting(struct commit *commit,
diff --git a/object.c b/object.c
index 4af3451..8e161f8 100644
--- a/object.c
+++ b/object.c
@@ -149,8 +149,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
struct tree *tree = lookup_tree(sha1);
if (tree) {
obj = &tree->object;
- if (!tree->buffer)
- tree->object.parsed = 0;
if (!tree->object.parsed) {
if (parse_tree_buffer(tree, buffer, size))
return NULL;
@@ -225,6 +223,29 @@ struct object *parse_object(const unsigned char *sha1)
return NULL;
}
+void free_object_buffer(struct object *item)
+{
+ if (!item)
+ return;
+
+ if (item->type == OBJ_TREE) {
+ struct tree *tree = (struct tree *)item;
+ tree->object.parsed = 0;
+ tree->size = 0;
+ if (tree->buffer) {
+ free(tree->buffer);
+ tree->buffer = NULL;
+ }
+ } else if (item->type == OBJ_COMMIT) {
+ struct commit *commit = (struct commit *)item;
+ commit->object.parsed = 0;
+ if (commit->buffer) {
+ free(commit->buffer);
+ commit->buffer = NULL;
+ }
+ }
+}
+
struct object_list *object_list_insert(struct object *item,
struct object_list **list_p)
{
diff --git a/object.h b/object.h
index 6a97b6b..cbc730c 100644
--- a/object.h
+++ b/object.h
@@ -63,6 +63,9 @@ struct object *parse_object(const unsigned char *sha1);
*/
struct object *parse_object_buffer(const unsigned char *sha1, enum object_type type, unsigned long size, void *buffer, int *eaten_p);
+/** If the object's type has a buffer, it's freed and marked as un-parsed. */
+void free_object_buffer(struct object *item);
+
/** Returns the object, with potentially excess memory allocated. **/
struct object *lookup_unknown_object(const unsigned char *sha1);
diff --git a/reachable.c b/reachable.c
index bf79706..c29d3e0 100644
--- a/reachable.c
+++ b/reachable.c
@@ -80,8 +80,7 @@ static void process_tree(struct tree *tree,
else
process_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);
}
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(obj);
}
static void process_tag(struct tag *tag, struct object_array *p,
diff --git a/revision.c b/revision.c
index 95d21e6..43c5eec 100644
--- a/revision.c
+++ b/revision.c
@@ -133,8 +133,7 @@ void mark_tree_uninteresting(struct tree *tree)
* We don't care about the tree any more
* after it has been marked uninteresting.
*/
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(obj);
}
void mark_parents_uninteresting(struct commit *commit)
diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..d7e26ca 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces correct results' '
test_cmp expect actual
'
+cat >expect <<EOF
+./helper other (refs/notes/textconv/magic)
+one
+EOF
+# add empty commit on master to make bug more reproducible
+test_expect_success 'pickaxe with cached textconv' '
+ git commit --allow-empty -m another &&
+ git log -S other --pretty=tformat:%s%d --all >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/upload-pack.c b/upload-pack.c
index 6142421..1feacbc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -81,8 +81,7 @@ static void show_commit(struct commit *commit, void *data)
die("broken output pipe");
fputc('\n', pack_pipe);
fflush(pack_pipe);
- free(commit->buffer);
- commit->buffer = NULL;
+ free_object_buffer(&commit->object);
}
static void show_object(struct object *obj,
diff --git a/walker.c b/walker.c
index be389dc..bae4876 100644
--- a/walker.c
+++ b/walker.c
@@ -56,10 +56,7 @@ static int process_tree(struct walker *walker, struct tree *tree)
if (!obj || process(walker, obj))
return -1;
}
- free(tree->buffer);
- tree->buffer = NULL;
- tree->size = 0;
- tree->object.parsed = 0;
+ free_object_buffer(&tree->object);
return 0;
}
--
1.8.1.1
Jonathon Mah
me@JonathonMah.com
^ permalink raw reply related
* Re: What's cooking in git.git (Jan 2013, #08; Tue, 22)
From: John Keeping @ 2013-01-23 21:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric S. Raymond, Chris Rorvick
In-Reply-To: <7vsj5rhlfs.fsf@alter.siamese.dyndns.org>
On Wed, Jan 23, 2013 at 09:13:27AM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > My preference would be for something like this, possibly with an
> > expanded examples section showing how to pipe the output of cvsps-3 or
> > cvs2git into git-fast-import:
> >
> > -- >8 --
> >
> > diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
> > index 9d5353e..20b846e 100644
> > --- a/Documentation/git-cvsimport.txt
> > +++ b/Documentation/git-cvsimport.txt
> > @@ -18,6 +18,11 @@ SYNOPSIS
> >
> > DESCRIPTION
> > -----------
> > +*WARNING:* `git cvsimport` uses cvsps version 2, which is considered
> > +deprecated; it does not work with cvsps version 3 and later. If you are
> > +performing a one-shot import of a CVS repository consider using cvsps-3,
> > +cvs2git or parsecvs directly.
> > +
> > Imports a CVS repository into git. It will either create a new
> > repository, or incrementally import into an existing one.
> >
> > -- 8< --
>
> OK, that is certainly a lot simpler to explain.
>
> Is it "it does not work yet with cvsps3", or "it will not ever work
> with cvsps3"? The impression I am getting is that it is the latter.
The existing script (git-cvsimport.perl) won't ever work with cvsps-3
since features it relies on have been removed.
> Also, should we have a suggestion to people who are *not* performing
> a one-shot import, i.e. doing incremental or bidirectional?
As far as I know cvsps is the only backend that attempts to support
partial exports but the support for that in its fast-export mode needs
work before I would consider it reliable. For now the existing
git-cvsimport is the best option I'm aware of.
John
^ permalink raw reply
* Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
From: Junio C Hamano @ 2013-01-23 21:06 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Nguyễn Thái Ngọc Duy, git, Heiko Voigt
In-Reply-To: <51004A37.6040301@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> This is a false positive. The merge algorithm picked a fast-forward
> in a submodule as a proper merge result and records that in a
> gitlink. But as Duy pointed out this could be easily fixed by
> turning the readonly flag off in that case.
I see that as "easily circumvented and not an effective protection",
though.
In theory, adding a gitlink to the index, removing a gitlink to the
index and modifying an existing gitlink in the index to another
gitlink in the index and writing the resulting in-core index out to
the on-disk index should be allowed, even after objects from the
submodule object database have contaminated our in-core object pool,
as long as you do not run cache_tree_update(). I am not sure if that
single loophole would be sufficient, though.
^ permalink raw reply
* Re: [PATCH v3 1/2] for-each-repo: new command used for multi-repo operations
From: Junio C Hamano @ 2013-01-23 20:54 UTC (permalink / raw)
To: Lars Hjemli; +Cc: git
In-Reply-To: <1358971180-10652-2-git-send-email-hjemli@gmail.com>
Lars Hjemli <hjemli@gmail.com> writes:
> diff --git a/Documentation/git-for-each-repo.txt b/Documentation/git-for-each-repo.txt
> new file mode 100644
> index 0000000..be49e96
> --- /dev/null
> +++ b/Documentation/git-for-each-repo.txt
> @@ -0,0 +1,62 @@
> +git-for-each-repo(1)
> +====================
> +
> +NAME
> +----
> +git-for-each-repo - Execute a git command in multiple repositories
"multiple non-bare repositories", I think.
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git for-each-repo' [--all|--clean|--dirty] [command]
> +
> +DESCRIPTION
> +-----------
> +The git-for-each-repo command is used to locate all git repositoris
Likewise; "all non-bare Git repositories".
> diff --git a/t/t6400-for-each-repo.sh b/t/t6400-for-each-repo.sh
> new file mode 100755
> index 0000000..4797629
> --- /dev/null
> +++ b/t/t6400-for-each-repo.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2013 Lars Hjemli
> +#
> +
> +test_description='Test the git-for-each-repo command'
> +
> +. ./test-lib.sh
> +
> +test_expect_success "setup" '
> + test_create_repo clean &&
> + (cd clean && test_commit foo) &&
> + git init --separate-git-dir=.cleansub clean/gitfile &&
> + (cd clean/gitfile && test_commit foo && echo bar >>foo.t) &&
> + test_create_repo dirty-wt &&
> + (cd dirty-wt && mv .git .linkedgit && ln -s .linkedgit .git &&
> + test_commit foo && rm foo.t) &&
> + test_create_repo dirty-idx &&
> + (cd dirty-idx && test_commit foo && git rm foo.t) &&
> + mkdir fakedir && mkdir fakedir/.git
> +'
> +
> +test_expect_success "without flags, all repos are included" '
> + echo "." >expect &&
> + echo "clean" >>expect &&
> + echo "clean/gitfile" >>expect &&
> + echo "dirty-idx" >>expect &&
> + echo "dirty-wt" >>expect &&
> + git for-each-repo | sort >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success "--dirty only includes dirty repos" '
> + echo "clean/gitfile" >expect &&
> + echo "dirty-idx" >>expect &&
> + echo "dirty-wt" >>expect &&
> + git for-each-repo --dirty | sort >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success "--clean only includes clean repos" '
> + echo "." >expect &&
> + echo "clean" >>expect &&
> + git for-each-repo --clean | sort >actual &&
> + test_cmp expect actual
> +'
Please add tests to show some command executions (e.g. test output
from "git ls-files", or something).
> +static void handle_repo(char *path, const char **argv)
> +{
> + if (path[0] == '.' && path[1] == '/')
> + path += 2;
> + if (match != ALL && match != get_repo_state())
> + return;
> + if (*argv) {
> + color_fprintf_ln(stdout, GIT_COLOR_YELLOW, "[%s]", path);
> + run_command_v_opt(argv, RUN_GIT_CMD);
This seems to allow people to run only a single Git subcommand,
which is probably not what most people want to see. Don't we want
to support something as simple as this?
git for-each-repository sh -c "ls *.c"
> + } else
> + printf("%s\n", path);
Assuming that the non *argv case is for consumption by programs and
scripts (similar to the way "ls-files" output is piped to downstream),
we prefer to (1) support "-z" so that "xargs -0" can read paths with
funny characters, and (2) use quote_c_style() from quote.c when "-z"
is not in effect.
> +}
> + ...
> + setenv(GIT_DIR_ENVIRONMENT, gitdir, 1);
> + strbuf_setlen(path, len - 1);
> + setenv(GIT_WORK_TREE_ENVIRONMENT, path->buf, 1);
> + handle_repo(path->buf, argv);
When you are only showing the path to a repository, I do not think
you want setenv() or chdir() at all. Shouldn't these be done inside
handle_repo() function? As you are only dealing with non-bare
repositories (and that is what you print in "listing only" mode
anyway), handle_repo() can borrow path (not path->buf) and append
and strip "/.git" as needed.
Also, while it is a good idea to protect this program from stray
GIT_DIR/GIT_WORK_TREE the user may have in the environment when this
program is started, I think this is not enough, if you allow the
*argv commands to run worktree related operations in each repository
you discover. You would need to chdir() to the top of the working
tree.
The run-command API lets you specify custom environment only for the
child process without affecting yourself by setting .env member of
the child_process structure, so we may want to use that instead of
doing setenv() on ourselves (and letting it inherited by the child).
^ permalink raw reply
* Re: [PATCHv2 6/8] submodule: simplify memory handling in config parsing
From: Jens Lehmann @ 2013-01-23 20:51 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jonathan Nieder, Joachim Schmitz,
René Scharfe, git
In-Reply-To: <20130123062642.GF5036@sigill.intra.peff.net>
Am 23.01.2013 07:26, schrieb Jeff King:
> We keep a strbuf for the name of the submodule, even though
> we only ever add one string to it. Let's just use xmemdupz
> instead, which is slightly more efficient and makes it
> easier to follow what is going on.
>
> Unfortunately, we still end up having to deal with some
> memory ownership issues in some code branches, as we have to
> allocate the string in order to do a string list lookup, and
> then only sometimes want to hand ownership of that string
> over to the string_list. Still, making that explicit in the
> code (as opposed to sometimes detaching the strbuf, and then
> always releasing it) makes it a little more obvious what is
> going on.
Thanks, this helps until I some day find the time to refactor
that code into a more digestible shape ;-)
Acked-by: Jens Lehmann <Jens.Lehmann@web.de>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> submodule.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 25413de..9ba1496 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)
> int parse_submodule_config_option(const char *var, const char *value)
> {
> struct string_list_item *config;
> - struct strbuf submodname = STRBUF_INIT;
> const char *name, *key;
> int namelen;
>
> @@ -135,37 +134,36 @@ int parse_submodule_config_option(const char *var, const char *value)
> return 0;
>
> if (!strcmp(key, "path")) {
> - strbuf_add(&submodname, name, namelen);
> config = unsorted_string_list_lookup(&config_name_for_path, value);
> if (config)
> free(config->util);
> else
> config = string_list_append(&config_name_for_path, xstrdup(value));
> - config->util = strbuf_detach(&submodname, NULL);
> - strbuf_release(&submodname);
> + config->util = xmemdupz(name, namelen);
> } else if (!strcmp(key, "fetchrecursesubmodules")) {
> - strbuf_add(&submodname, name, namelen);
> - config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);
> + char *name_cstr = xmemdupz(name, namelen);
> + config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name_cstr);
> if (!config)
> - config = string_list_append(&config_fetch_recurse_submodules_for_name,
> - strbuf_detach(&submodname, NULL));
> + config = string_list_append(&config_fetch_recurse_submodules_for_name, name_cstr);
> + else
> + free(name_cstr);
> config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);
> - strbuf_release(&submodname);
> } else if (!strcmp(key, "ignore")) {
> + char *name_cstr;
> +
> if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
> strcmp(value, "all") && strcmp(value, "none")) {
> warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
> return 0;
> }
>
> - strbuf_add(&submodname, name, namelen);
> - config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);
> - if (config)
> + name_cstr = xmemdupz(name, namelen);
> + config = unsorted_string_list_lookup(&config_ignore_for_name, name_cstr);
> + if (config) {
> free(config->util);
> - else
> - config = string_list_append(&config_ignore_for_name,
> - strbuf_detach(&submodname, NULL));
> - strbuf_release(&submodname);
> + free(name_cstr);
> + } else
> + config = string_list_append(&config_ignore_for_name, name_cstr);
> config->util = xstrdup(value);
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCHv2 5/8] submodule: use parse_config_key when parsing config
From: Jens Lehmann @ 2013-01-23 20:45 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Jonathan Nieder, Joachim Schmitz,
René Scharfe, git
In-Reply-To: <20130123062522.GE5036@sigill.intra.peff.net>
Am 23.01.2013 07:25, schrieb Jeff King:
> This makes the code a lot simpler to read by dropping a
> whole bunch of constant offsets.
>
> As a bonus, it means we also feed the whole config variable
> name to our error functions:
>
> [before]
> $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout
> fatal: bad foo.fetchrecursesubmodules argument: bogus
>
> [after]
> $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout
> fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus
Thanks, that makes lots of sense!
Acked-by: Jens Lehmann <Jens.Lehmann@web.de>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> submodule.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 2f55436..25413de 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -126,15 +126,16 @@ int parse_submodule_config_option(const char *var, const char *value)
>
> int parse_submodule_config_option(const char *var, const char *value)
> {
> - int len;
> struct string_list_item *config;
> struct strbuf submodname = STRBUF_INIT;
> + const char *name, *key;
> + int namelen;
>
> - var += 10; /* Skip "submodule." */
> + if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
> + return 0;
>
> - len = strlen(var);
> - if ((len > 5) && !strcmp(var + len - 5, ".path")) {
> - strbuf_add(&submodname, var, len - 5);
> + if (!strcmp(key, "path")) {
> + strbuf_add(&submodname, name, namelen);
> config = unsorted_string_list_lookup(&config_name_for_path, value);
> if (config)
> free(config->util);
> @@ -142,22 +143,22 @@ int parse_submodule_config_option(const char *var, const char *value)
> config = string_list_append(&config_name_for_path, xstrdup(value));
> config->util = strbuf_detach(&submodname, NULL);
> strbuf_release(&submodname);
> - } else if ((len > 23) && !strcmp(var + len - 23, ".fetchrecursesubmodules")) {
> - strbuf_add(&submodname, var, len - 23);
> + } else if (!strcmp(key, "fetchrecursesubmodules")) {
> + strbuf_add(&submodname, name, namelen);
> config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);
> if (!config)
> config = string_list_append(&config_fetch_recurse_submodules_for_name,
> strbuf_detach(&submodname, NULL));
> config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);
> strbuf_release(&submodname);
> - } else if ((len > 7) && !strcmp(var + len - 7, ".ignore")) {
> + } else if (!strcmp(key, "ignore")) {
> if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
> strcmp(value, "all") && strcmp(value, "none")) {
> warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
> return 0;
> }
>
> - strbuf_add(&submodname, var, len - 7);
> + strbuf_add(&submodname, name, namelen);
> config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);
> if (config)
> free(config->util);
>
^ permalink raw reply
* Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
From: Jens Lehmann @ 2013-01-23 20:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git, Heiko Voigt
In-Reply-To: <7v1udbj0kt.fsf@alter.siamese.dyndns.org>
Am 23.01.2013 18:01, schrieb Junio C Hamano:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> add_submodule_odb() can be used to import objects from another
>> repository temporarily. After this point we don't know which objects
>> are ours, which are external. If we create an object that refers to an
>> external object, next time git runs, it may find a hole in the object
>> graph because the external repository may not be imported. The same
>> goes for pointing a ref to an external SHA-1.
>>
>> To protect ourselves, once add_submodule_odb() is used:
>>
>> - trees, tags and commits cannot be created
>> - refs cannot be updated
>>
>> In certain cases that submodule code knows that it's safe to write, it
>> can turn the readonly flag off.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> I think this is a good safety check.
>
> Two step implementation to bring "read-only" support into a testable
> shape and then flip that bit in add_submdule_odb() would be a
> sensible approach.
I agree this is a worthwhile change so nobody accidentally screws
things up.
>> It catches at least a case in
>> t7405.3. I did not investigate further though.
This is a false positive. The merge algorithm picked a fast-forward
in a submodule as a proper merge result and records that in a
gitlink. But as Duy pointed out this could be easily fixed by
turning the readonly flag off in that case.
> I however have this suspicion that this will become a losing battle
> and we would be better off getting rid of add_submodule_odb();
> instead operations that work across repositories will be done as a
> subprocess, which will get us back closer to one of the original
> design goals of submodule support to have a clear separation between
> the superproject and its submodules.
Please don't. While I agree with your goal, I'd be unhappy to do
that because of the performance drop (especially on fork-challenged
operating systems).
^ permalink raw reply
* Re: [PATCH v3 2/8] git_remote_helpers: fix input when running under Python 3
From: Junio C Hamano @ 2013-01-23 20:36 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Dennis Kaarsemaker, John Keeping, Git List
In-Reply-To: <CAGdFq_jZDUxg7oTL7Z4v5ezYFPfJ8kZR6iHpESw6WnoDCeAy8w@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> writes:
> On Wed, Jan 23, 2013 at 11:47 AM, John Keeping <john@keeping.me.uk> wrote:
>>> When did we last revisit what minimal python version we are ok with requiring?
>>
>> I was wondering if people would weigh in discussing that in response to
>> [1] but no one has commented on that part of it. As another datapoint,
>> Brandon Casey was suggesting patching git-p4.py to support Python 2.4
>> [2].
>>
>> [1] http://article.gmane.org/gmane.comp.version-control.git/213920
>> [2] http://article.gmane.org/gmane.comp.version-control.git/214048
>
> I for one would be happy to kill off support for anything older than
> 2.6 (which had it's latest release on October 1st, 2008).
>
> Junio, how have we decided in the past which version of x to support?
I do not think there was any conclusion. $gmane/212215 claiming 2.4
support matters for RHEL 5.x users was the last on the topic as far
as I can tell, so it boils down to another question: do users on
RHEL 5.x matter?
I can read from $gmane/212215 that users of the said platform can
safely keep using Python 2.4 under their vendor support contract
until 2017. But let's focus on what do these users expect of their
system and software they run on it a bit.
When they want to run a piece software that is not shipped with
RHEL, either by writing their own or by importing from elsewhere,
that needs 2.6 features, what are their options?
(a) The platform vendor optionally supplies 2.6 with or without
support;
(b) The users can and do install 2.6 as /usr/local/bin/python2.6,
which may even be community-supported, but the vendor does not
support it; or
(c) The vendor terminates the support contract for users who choose
to go (b).
I think we can safely discard (c); if that is the case, the users on
the said platform will not choose to update Git either, so it does
not matter where the future versions of Git sets the lower bound of
Python version at.
If we are not talking about the situation (c), then the users can
choose to use 2.6, and more importantly, Python being a popular
software, I would imagine that there are reputable sources of
prepackaged RPMs for them to do so without going too much hassle of
configuring, compiling and installing.
Now how does the decision we make today for releases of Git that
haven't yet happened will affect these users? As these versions of
newer Git were not shipped with RHEL 5.x, and also I am assuming
that Git is a more niche product than Python is, I would imagine
that it is very unlikely that the vendor gives it the users as an
optional package. The users will have to do the same thing to be
able to use such versions of Git as whatever they do in order to use
Python 2.6.
Given that, what the vendor originally shipped and officially
supports does not affect the choices we would make today for newer
versions of Git. The users in a shop where additional third-party
software in /usr/local/bin is strictly forbidden, they are stuck
with the version of Git that the vendor shipped anyway, because they
won't be able to install an updated Git in /usr/local/bin, either.
That is, unless installing 2.6 as /usr/local/bin/python2.6 (or if
you are really paranoid, /usr/local/only-for-git/bin/python2.6 where
nobody's $PATH points at) is impossible.
So personally I do not think dropping 2.4 is a huge problem for
future versions of Git, but I'd like to hear from those working in
IT support for large and slow-moving organizations (aka RHEL 5
customers).
^ permalink raw reply
* Re: Question re. git remote repository
From: Junio C Hamano @ 2013-01-23 20:24 UTC (permalink / raw)
To: Lang, David; +Cc: 'Matt Seitz', David Lang, git@vger.kernel.org
In-Reply-To: <201301231941.r0NJf3oa001238@smtpb01.one-mail.on.ca>
"Lang, David" <David.Lang@uhn.ca> writes:
> Thanks Matt and Dave and everyone else for your feedback on this.
[administrivia: please wrap your lines to reasonable length]
> 1. Download and install git for Windows on the 2 networked developer's
> PC's and the 1 networked server.
>
> 2. On the server...
> A) Initialize the Visual Studio folder for a particular
> project as a git repository using 'git init'
> b) Using the git rep just created (above), create a bare
> repository on the server to act as the remote/master repository using
> git clone --bare'
optionally:
C) remove the original directory (A)
D) make a non-bare clone on the server with "git clone", if
you would like to have a single build environment on the
server box.
E) Use "git pull" from the bare repository you created in
step (2.B) to update the repository you created in step
(2.D) as necessary in order to build the latest in this
repository.
> 3. On each of the PC's...
> A) Clone the remote repository from the network server using
> git clone' (this will automatically create 'origin' as a remote source
> on the PC's)
B) Each developer works in his repository; use either "git
pull" or "git pull --rebase" to sync up with the tip of
the master repository as necessary;
C) When a developer's work reaches a point where it is good
enough to update the master repository, use "git push" to
update the bare repository you created on the server in
step (2.B). This may need to trigger step (2.E).
> Couple of questions...
> ...
> 4. The original Visual Studio project folder essentially remains
> untouched, correct? The 'git init' and 'git clone' commands just make
> copies and references of whatever data is in the VS project folder,
> right?
These operations make copies and after making copies they do not
ever refer to the original, so you can take a back-up of the
original and remove it (i.e. optional step (c)).
^ permalink raw reply
* Re: [PATCH v2] all: new command used for multi-repo operations
From: Jens Lehmann @ 2013-01-23 20:17 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Lars Hjemli, git
In-Reply-To: <CACsJy8DskoCi9Lg+HW0JeQBe4HX-bMXNHUgfrsg+DoqBN9-ntQ@mail.gmail.com>
Am 23.01.2013 09:55, schrieb Duy Nguyen:
> On Wed, Jan 23, 2013 at 3:12 PM, Lars Hjemli <hjemli@gmail.com> wrote:
>> +NAME
>> +----
>> +git-all - Execute a git command in multiple repositories
>
> I agree with Junio "git-all" is too generic.
+1
>> +static int get_repo_state()
>> +{
>> + const char *diffidx[] = {"diff", "--quiet", "--cached", NULL};
>> + const char *diffwd[] = {"diff", "--quiet", NULL};
>> +
>> + if (run_command_v_opt(diffidx, RUN_GIT_CMD) != 0)
>> + return DIRTY;
>> + if (run_command_v_opt(diffwd, RUN_GIT_CMD) != 0)
>> + return DIRTY;
>> + return CLEAN;
>> +}
>
> Perhaps we could add the subrepo's object data to the in-memory object
> database of git-all, then do the diff without launching new commands?
You could do that for the "--cached" case, but not for the plain diff.
But I think forking a "git status --porcelain -uno" and testing if it
produced any output should do the trick with a single fork.
^ permalink raw reply
* Re: [PATCH v3 2/8] git_remote_helpers: fix input when running under Python 3
From: Sverre Rabbelier @ 2013-01-23 20:14 UTC (permalink / raw)
To: John Keeping, Junio C Hamano; +Cc: Git List
In-Reply-To: <20130123194757.GQ7498@serenity.lan>
On Wed, Jan 23, 2013 at 11:47 AM, John Keeping <john@keeping.me.uk> wrote:
>> When did we last revisit what minimal python version we are ok with requiring?
>
> I was wondering if people would weigh in discussing that in response to
> [1] but no one has commented on that part of it. As another datapoint,
> Brandon Casey was suggesting patching git-p4.py to support Python 2.4
> [2].
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/213920
> [2] http://article.gmane.org/gmane.comp.version-control.git/214048
I for one would be happy to kill off support for anything older than
2.6 (which had it's latest release on October 1st, 2008).
Junio, how have we decided in the past which version of x to support?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: segmentation fault (nullpointer) with git log --submodule -p
From: Jeff King @ 2013-01-23 20:02 UTC (permalink / raw)
To: Armin; +Cc: git
In-Reply-To: <20130123143816.GA579@krypton.darkbyte.org>
On Wed, Jan 23, 2013 at 03:38:16PM +0100, Armin wrote:
> Hello dear git people.
>
> I experience a reproducible segmentation fault on one of my
> repositories when doing a "git log --submodule -p", tested with newest
> version on Arch Linux (git version 1.8.1.1) and built fresh (git
> version 1.8.1.1.347.g9591fcc), tried on 2 seperate systems:
>
>
> Program terminated with signal 11, Segmentation fault.
> #0 0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752
> 752 for (i = 0; msg[i]; i++) {
> [...]
> (gdb) l
> 747 static void parse_commit_header(struct format_commit_context *context)
> 748 {
> 749 const char *msg = context->message;
> 750 int i;
> 751
> 752 for (i = 0; msg[i]; i++) {
> 753 int eol;
> 754 for (eol = i; msg[eol] && msg[eol] != '\n'; eol++)
> 755 ; /* do nothing */
> 756
> (gdb) p msg
> $7 = <optimized out>
> (gdb) p context->message
> $8 = 0x0
Yeah, that should definitely not be NULL. I can't reproduce here with a
few simple examples, though.
Does it fail with older versions of git? If so, can you bisect?
Is it possible for you to make your repo available?
-Peff
^ permalink raw reply
* Re: [PATCH] Ignore gitk-wish buildproduct
From: Junio C Hamano @ 2013-01-23 20:00 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Lars Hjemli, git
In-Reply-To: <7vip6nhdry.fsf@alter.siamese.dyndns.org>
From: Christian Couder <chriscool@tuxfamily.org>
gitk, when bound into the git.git project tree, used to live at the
root level, but in 62ba514 (Move gitk to its own subdirectory,
2007-11-17) it was moved to a subdirectory. The code used to track
changes to TCLTK_PATH (which should cause gitk to be rebuilt to
point at the new interpreter) was left in the main Makefile instead
of being moved to the new Makefile that was created for the gitk
project.
Also add .gitignore file to list build artifacts for the gitk
project.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Paul, this is relative to the tip of your tree, 386befb (gitk:
Display important heads even when there are many, 2013-01-02).
Could you consider applying it?
Also I notice that you have many patches I still do not have
there, and I'd appreciate a "Go ahead and pull from me!".
Thanks.
.gitignore | 2 ++
Makefile | 16 ++++++++++++++--
2 files changed, 16 insertions(+), 2 deletions(-)
create mode 100644 .gitignore
diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..d7ebcaf
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,2 @@
+/GIT-TCLTK-VARS
+/gitk-wish
diff --git a/Makefile b/Makefile
index e1b6045..5acdc90 100644
--- a/Makefile
+++ b/Makefile
@@ -17,6 +17,16 @@ DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
bindir_SQ = $(subst ','\'',$(bindir))
TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
+### Detect Tck/Tk interpreter path changes
+TRACK_TCLTK = $(subst ','\'',-DTCLTK_PATH='$(TCLTK_PATH_SQ)')
+
+GIT-TCLTK-VARS: FORCE
+ @VARS='$(TRACK_TCLTK)'; \
+ if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
+ echo 1>&2 " * new Tcl/Tk interpreter location"; \
+ echo "$$VARS" >$@; \
+ fi
+
## po-file creation rules
XGETTEXT ?= xgettext
ifdef NO_MSGFMT
@@ -49,9 +59,9 @@ uninstall::
$(RM) '$(DESTDIR_SQ)$(bindir_SQ)'/gitk
clean::
- $(RM) gitk-wish po/*.msg
+ $(RM) gitk-wish po/*.msg GIT-TCLTK-VARS
-gitk-wish: gitk
+gitk-wish: gitk GIT-TCLTK-VARS
$(QUIET_GEN)$(RM) $@ $@+ && \
sed -e '1,3s|^exec .* "$$0"|exec $(subst |,'\|',$(TCLTK_PATH_SQ)) "$$0"|' <gitk >$@+ && \
chmod +x $@+ && \
@@ -65,3 +75,5 @@ $(ALL_MSGFILES): %.msg : %.po
@echo Generating catalog $@
$(MSGFMT) --statistics --tcl $< -l $(basename $(notdir $<)) -d $(dir $@)
+.PHONY: all install uninstall clean update-po
+.PHONY: FORCE
--
1.8.1.336.g866ceff
^ permalink raw reply related
* [PATCH v3 2/2] git: rewrite `git -a` to become a git-for-each-repo command
From: Lars Hjemli @ 2013-01-23 19:59 UTC (permalink / raw)
To: git; +Cc: Lars Hjemli
In-Reply-To: <1358971180-10652-1-git-send-email-hjemli@gmail.com>
With this rewriting, it is now possible to run e.g. `git -ad gui` to
start up git-gui in each repo within the current directory which
contains uncommited work.
Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
git.c | 36 ++++++++++++++++++++++++++++++++++++
t/t6400-for-each-repo.sh | 6 ++++++
2 files changed, 42 insertions(+)
diff --git a/git.c b/git.c
index 6b53169..f933b5d 100644
--- a/git.c
+++ b/git.c
@@ -31,8 +31,42 @@ static void commit_pager_choice(void) {
}
}
+/*
+ * Rewrite 'git -ad status' to 'git for-each-repo -d status'
+ */
+static int rewrite_foreach_repo(const char ***orig_argv,
+ const char **curr_argv,
+ int *curr_argc)
+{
+ const char **new_argv;
+ char *tmp;
+ int new_argc, curr_pos, i, j;
+
+ curr_pos = curr_argv - *orig_argv;
+ if (strlen(curr_argv[0]) == 2) {
+ curr_argv[0] = "for-each-repo";
+ return curr_pos - 1;
+ }
+
+ new_argc = curr_pos + *curr_argc + 1;
+ new_argv = xmalloc(new_argc * sizeof(void *));
+ for (i = j = 0; j < new_argc; i++, j++) {
+ if (i == curr_pos) {
+ asprintf(&tmp, "-%s", (*orig_argv)[i] + 2);
+ new_argv[j] = "for-each-repo";
+ new_argv[++j] = tmp;
+ } else {
+ new_argv[j] = (*orig_argv)[i];
+ }
+ }
+ *orig_argv = new_argv;
+ (*curr_argc)++;
+ return curr_pos;
+}
+
static int handle_options(const char ***argv, int *argc, int *envchanged)
{
+ const char ***pargv = argv;
const char **orig_argv = *argv;
while (*argc > 0) {
@@ -143,6 +177,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
setenv(GIT_LITERAL_PATHSPECS_ENVIRONMENT, "0", 1);
if (envchanged)
*envchanged = 1;
+ } else if (!strncmp(cmd, "-a", 2)) {
+ return rewrite_foreach_repo(pargv, *argv, argc);
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
usage(git_usage_string);
diff --git a/t/t6400-for-each-repo.sh b/t/t6400-for-each-repo.sh
index 4797629..b501605 100755
--- a/t/t6400-for-each-repo.sh
+++ b/t/t6400-for-each-repo.sh
@@ -27,6 +27,8 @@ test_expect_success "without flags, all repos are included" '
echo "dirty-idx" >>expect &&
echo "dirty-wt" >>expect &&
git for-each-repo | sort >actual &&
+ test_cmp expect actual &&
+ git -a | sort >actual &&
test_cmp expect actual
'
@@ -35,6 +37,8 @@ test_expect_success "--dirty only includes dirty repos" '
echo "dirty-idx" >>expect &&
echo "dirty-wt" >>expect &&
git for-each-repo --dirty | sort >actual &&
+ test_cmp expect actual &&
+ git -ad | sort >actual &&
test_cmp expect actual
'
@@ -42,6 +46,8 @@ test_expect_success "--clean only includes clean repos" '
echo "." >expect &&
echo "clean" >>expect &&
git for-each-repo --clean | sort >actual &&
+ test_cmp expect actual &&
+ git -ac | sort >actual &&
test_cmp expect actual
'
--
1.8.1.1.350.g3346805
^ permalink raw reply related
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