* Re: [PATCH v4 3/3] upload-archive: use start_command instead of fork
From: Thomas Rast @ 2011-11-15 10:22 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git, gitster, j6t, peff, rene.scharfe
In-Reply-To: <1319472131-3968-4-git-send-email-kusmabite@gmail.com>
Erik Faye-Lund wrote:
> The POSIX-function fork is not supported on Windows. Use our
> start_command API instead.
>
> As this is the last call-site that depends on the fork-stub in
> compat/mingw.h, remove that as well.
>
> Add an undocumented flag to git-archive that tells it that the
> action originated from a remote, so features can be disabled.
> Thanks to Jeff King for work on this part.
>
> Remove the NOT_MINGW-prereq for t5000, as git-archive --remote
> now works.
I see valgrind failures bisecting to this commit, like so:
==19125== Syscall param execve(argv[i]) points to unaddressable byte(s)
==19125== at 0x5303CB7: execve (in /lib64/libc-2.11.3.so)
==19125== by 0x53045A5: execvpe (in /lib64/libc-2.11.3.so)
==19125== by 0x4B183C: execv_git_cmd (exec_cmd.c:137)
==19125== by 0x4F305E: start_command (run-command.c:277)
==19125== by 0x47F5C9: cmd_upload_archive (upload-archive.c:98)
==19125== by 0x4051F4: run_builtin (git.c:308)
==19125== by 0x40538F: handle_internal_command (git.c:466)
==19125== by 0x405556: main (git.c:553)
==19125== Address 0x7feffe7d0 is not stack'd, malloc'd or (recently) free'd
when running 'make valgrind' in current master. Let me know if you
need more information.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH] revert: prettify fatal messages
From: Matthieu Moy @ 2011-11-15 10:04 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano, Jonathan Nieder
In-Reply-To: <1321349492-5653-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Some of the fatal messages printed by revert and cherry-pick look ugly
> like the following:
>
> fatal: Could not open .git/sequencer/todo.: No such file or directory
>
> The culprit here is the die_errno() function that takes a custom error
> message string as an argument and appends ": <message from errno>"
> before printing it. So, we can fix the problem by not terminating the
> string argument to the function with a '.' (period). Fatal messages
> look nicer now:
>
> fatal: Could not open .git/sequencer/todo: No such file or directory
Sounds (obviously) good.
(I just misread the subject line at first, and understood that this was
a revert of an earlier commit, while this is actually a commit touching
the "revert" part of Git)
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 3/5] sequencer: sequencer state is useless without todo
From: Jonathan Nieder @ 2011-11-15 9:52 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <CALkWK0nUuzn2_itdACHLQBpUaVv97tFAjNGdVBEhWC7a6Rp75w@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Yeah, git_path() writes to one of the four static buffers in
> path.c:get_pathname(). Which brings me to: what should (can) we do
> about it?
Just use a sane idiom. Which means: as few git_path() values in
flight at a time as possible.
In other words, do not save the git_path() result in a variable, but
pass it directly to whatever computation needs to use it.
^ permalink raw reply
* Re: [PATCH 1/5] sequencer: factor code out of revert builtin
From: Jonathan Nieder @ 2011-11-15 9:47 UTC (permalink / raw)
To: Miles Bader; +Cc: Ramkumar Ramachandra, Junio C Hamano, Git List
In-Reply-To: <buo4ny5u4k6.fsf@dhlpc061.dev.necel.com>
Miles Bader wrote:
> Does git not use the common practice of self-contained headers?
It usually does, with two exceptions.
Headers do not usually include git-compat-util.h directly, which is a
good thing, since it reminds callers to include git-compat-util.h
before anything else.
Headers might sometimes forget to declare types defined in cache.h,
which would be a mistake. For example, in branch.h we see:
int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);
Which means the following code does not type-check:
#include "git-compat-util.h"
#include "branch.h"
#include "strbuf.h"
int demo(const char *name, struct strbuf *ref)
{
return validate_new_branchname(name, ref, 0, 0);
}
Reordering the #includes to put strbuf.h before branch.h is a possible
workaround. Adding the missing forward declaration is better:
diff --git i/branch.h w/branch.h
index 1285158d..d5240a20 100644
--- i/branch.h
+++ w/branch.h
@@ -1,6 +1,9 @@
#ifndef BRANCH_H
#define BRANCH_H
+struct strbuf;
+enum branch_track;
+
/* Functions for acting on the information about branches. */
/*
--
^ permalink raw reply related
* [PATCH] revert: prettify fatal messages
From: Ramkumar Ramachandra @ 2011-11-15 9:31 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
Some of the fatal messages printed by revert and cherry-pick look ugly
like the following:
fatal: Could not open .git/sequencer/todo.: No such file or directory
The culprit here is the die_errno() function that takes a custom error
message string as an argument and appends ": <message from errno>"
before printing it. So, we can fix the problem by not terminating the
string argument to the function with a '.' (period). Fatal messages
look nicer now:
fatal: Could not open .git/sequencer/todo: No such file or directory
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Candidate for 'maint'?
builtin/revert.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 87df70e..e0319c8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -331,7 +331,7 @@ static void write_message(struct strbuf *msgbuf, const char *filename)
int msg_fd = hold_lock_file_for_update(&msg_file, filename,
LOCK_DIE_ON_ERROR);
if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
- die_errno(_("Could not write to %s."), filename);
+ die_errno(_("Could not write to %s"), filename);
strbuf_release(msgbuf);
if (commit_lock_file(&msg_file) < 0)
die(_("Error wrapping up %s"), filename);
@@ -767,7 +767,7 @@ static void read_populate_todo(struct commit_list **todo_list,
fd = open(todo_file, O_RDONLY);
if (fd < 0)
- die_errno(_("Could not open %s."), todo_file);
+ die_errno(_("Could not open %s"), todo_file);
if (strbuf_read(&buf, fd, 0) < 0) {
close(fd);
strbuf_release(&buf);
@@ -845,7 +845,7 @@ static int create_seq_dir(void)
if (file_exists(seq_dir))
return error(_("%s already exists."), seq_dir);
else if (mkdir(seq_dir, 0777) < 0)
- die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
+ die_errno(_("Could not create sequencer directory %s"), seq_dir);
return 0;
}
@@ -859,7 +859,7 @@ static void save_head(const char *head)
fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
strbuf_addf(&buf, "%s\n", head);
if (write_in_full(fd, buf.buf, buf.len) < 0)
- die_errno(_("Could not write to %s."), head_file);
+ die_errno(_("Could not write to %s"), head_file);
if (commit_lock_file(&head_lock) < 0)
die(_("Error wrapping up %s."), head_file);
}
@@ -876,7 +876,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
die(_("Could not format %s."), todo_file);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
strbuf_release(&buf);
- die_errno(_("Could not write to %s."), todo_file);
+ die_errno(_("Could not write to %s"), todo_file);
}
if (commit_lock_file(&todo_lock) < 0) {
strbuf_release(&buf);
--
1.7.6.351.gb35ac.dirty
^ permalink raw reply related
* Re: [PATCH 1/5] sequencer: factor code out of revert builtin
From: Miles Bader @ 2011-11-15 9:18 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Jonathan Nieder, Git List
In-Reply-To: <CALkWK0mtmRYyFosQNJixhheUmHpRjWc4A5zPQ6AaBfmw4H4eLQ@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Junio C Hamano writes:
>> [...]
>> With that observation, it would probably make more sense if "foo.c"
>> included the headers in the following order:
>>
>> - Anything the declarations in "foo.h" depends on;
>> - "foo.h" itself; and finally
>> - Other headers that "foo.c" implementation needs.
>>
>> That way, people who want to use "foo.h" can guess what needs to be
>> included before using "foo.h" a lot more easily.
>
> That's a good rule-of-thumb. Thanks :)
Does git not use the common practice of self-contained headers?
-miles
--
Fast, small, soon; pick any 2.
^ permalink raw reply
* Re: [PATCH 3/5] sequencer: sequencer state is useless without todo
From: Ramkumar Ramachandra @ 2011-11-15 9:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Git List
In-Reply-To: <7v7h33oifq.fsf@alter.siamese.dyndns.org>
Hi,
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>>>> static int create_seq_dir(void)
>>>> {
>>>> + const char *todo_file = git_path(SEQ_TODO_FILE);
>>>> const char *seq_dir = git_path(SEQ_DIR);
>>>
>>> Scary idiom.
>>
>> What's scary about it?
>
> The next person who copies and pastes this code to other codepaths without
> thinking that the return value of git_path() is ephemeral and may need to
> be saved away depending on what goes between its assignment and its use.
Yeah, git_path() writes to one of the four static buffers in
path.c:get_pathname(). Which brings me to: what should (can) we do
about it? Explicitly xmalloc()'ing and free()'ing a tiny path buffer
is an overkill, so I'm thinking more on the lines of good
documentation. I've been guilty of misusing git_path() blindly in the
past myself.
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 1/5] sequencer: factor code out of revert builtin
From: Ramkumar Ramachandra @ 2011-11-15 9:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Git List
In-Reply-To: <7vvcqnmxeu.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano writes:
> [...]
> With that observation, it would probably make more sense if "foo.c"
> included the headers in the following order:
>
> - git-compat-util.h (or the prominent ones like "cache.h" that is known
> to include it at the beginning);
> - Anything the declarations in "foo.h" depends on;
> - "foo.h" itself; and finally
> - Other headers that "foo.c" implementation needs.
>
> That way, people who want to use "foo.h" can guess what needs to be
> included before using "foo.h" a lot more easily.
That's a good rule-of-thumb. Thanks :)
-- Ram
^ permalink raw reply
* Re: [PATCH 3/7] sequencer: handle single-commit pick as special case
From: Ramkumar Ramachandra @ 2011-11-15 8:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <7vpqgvmwtl.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> [...]
> [...]
> I do not see an inconsistency here, let alone any "glaring" one.
Yeah, my commit message is totally misleading and unclear. Yes, all
the operations make sense to you and me; for a new user who doesn't
understand how Git works, it's completely inconsistent at a very
superficial level, no? The sequence of operations he has to execute
depends on:
1. If he literally specified a single commit or a commit range on the
command-line.
2. In the case of multi-commit picking, there's an additional layer of
decision making: did the conflict occur in the last commit in the
range?
Anyway, I'll rewrite this commit message in the next iteration.
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 0/7] New sequencer workflow!
From: Ramkumar Ramachandra @ 2011-11-15 8:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <7vaa7zmuwt.fsf@alter.siamese.dyndns.org>
Hi Junio,
Thanks for the fantastic review. My lack of foresight is very disturbing.
Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> This series preserves the old workflow:
>> [...]
> Hmm, doesn't "git add && git cherry-pick --continue" (i.e. no "git commit"
> in between) trigger the "commit the resolution first and then go on"?
Ugh, right. I can't possibly preserve the old workflow fully- what I
meant is that users can still "git commit" before continuing (in the
case of a multi-commit operation). It's not so much about preserving
a workflow, as it is about not breaking existing tests.
> Almost. If these are replaced with "git cherry-pick --continue" and "git
> revert --continue" that internally triggers "git sequencer --continue"
> *and* errors out if a wrong command was used to "--continue", it would be
> perfect.
Okay, there are two ways to fix this:
1. If the first instruction in the instruction sheet is a "revert" and
"git cherry-pick --continue" is called or viceversa, error out. The
downside is that there's really no sane way to handle mixed "pick" and
"revert" instructions in the sheet using this approach: parsing the
whole sheet in advance is a total overkill.
2. The key issue is that the sequencer doesn't keep track of which
command was actually executed (argv[0]): fix this by creating a new
'.git/sequencer/cmd' to keep this information.
I think we should pick the latter option. Also, why should "git
<cherry-pick|revert> --continue" invoke "git sequencer --continue"
when it can just call into the corresponding function in the sequencer
library (isn't lib'ification one of the issues we're trying to
address)?
> While I do not mind "sequencer --continue" as a step to get us closer to a
> more pleasant user experience at the implementation level, exposing the
> name "sequencer" or having the user to type "sequencer --continue" is going
> in a very wrong direction at the UI level.
> [...]
> Now, if you rename "cherry-pick --continue" and "revert --continue" to
> "sequencer --continue", what message are you sending to the users? They
> now need to learn that only these two commands are based on something
> called "sequencer", can be resumed with "sequencer --continue", but other
> commands need to be continued with "X --continue".
Thanks for the superb explanation- it's plain as day now. In a
nutshell: it makes little sense to have a grand plan about an
all-encompassing "git sequencer --continue"; even if that is the plan
in the super-long term (although "git continue" is a better idea), it
makes no sense to burden the user now with additional UI
inconsistencies.
I'm still a little confused about what to do with the "git sequencer"
builtin though: how do we prevent users from being confused by it- by
not advertising it?
> The original workflow was "pull; edit && add && commit", and it is very
> unlikely this will change while we are still in Git 1.X series. The
> original single commit variants of "cherry-pick" and "revert" are in the
> same category. We would need to keep supporting "cherry-pick/revert; edit
> && add && commit" as a workflow for a while.
True. I wrote this pretending that only "git <cherry-pick|revert>"
was stuck with this UI inconsistency between one-commit picks and
many-commit picks.
> In the shorter term, a person new to Git, after learning "run command X, X
> gives back control asking for help, help X by editing and adding, and
> telling X to continue" pattern from these commands, will eventually find
> that the commands in single stop-point category (i.e. "pull", "merge" and
> single-commit "cherry-pick" and "revert") inconsistent from others that
> take "--continue" to resume. For this reason, "git cherry-pick --continue"
> that would work even when picking a single commit like this would be
> beneficial:
> [...]
Interesting aside: this final detail is fixed only in the last patch
in the series; that's how deep the problem is.
> In the longer term (now we are talking about Git 2.X version bump), it
> would be ideal if all the "git X --continue" can be consolidated to a
> single "git continue" command (and "git abort" to give up the sequence
> of operations).
>
> Given that bash-completion script can tell in what state the repository
> is, I think this is doable. "git continue" may invoke your implementation
> of "git sequencer --continue" internally when it detects that the state is
> something the "sequencer" machinery knows how to continue, and if it is in
> the middle of conflicted "am -3" or rejected "am", the command may invoke
> "git am --continue" instead.
Makes perfect sense.
Thanks.
-- Ram
^ permalink raw reply
* Re: Git 1.7.7.1 (Win7) work slower.
From: Junio C Hamano @ 2011-11-15 8:08 UTC (permalink / raw)
To: mardu; +Cc: git
In-Reply-To: <49277.1321343947@aster.pl>
mardu@aster.pl writes:
> The latest Git version for Windows (1.7.7.1; GUI) work very slow (it means that I
> have to wait tens seconds or even few minutes for open the application, scan
> files, read history and other...).
> I don't see changes in speed in another aplications installed on my computer.
> The previous version Git (1.7.6), which I had installed, it has worked normaly.
>
> OS:Windows 7 x64 (Home Premium, SP1),
> CPU: Intel Core i5 M520 2.40GHz
> RAM: 4GB
Wrong mailing list? I suspect you meant "msysGit <msysgit@googlegroups.com>".
^ permalink raw reply
* Re: [PATCH] i18n: add infrastructure for translating Git with gettext
From: Junio C Hamano @ 2011-11-15 8:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jakub Narebski, Jeff Epler, Jonathan Nieder, Johannes Sixt,
Erik Faye-Lund, Thomas Rast, Peter Krefting
In-Reply-To: <1321191835-24062-1-git-send-email-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Change the skeleton implementation of i18n in Git to one that can show
> localized strings to users for our C, Shell and Perl programs using
> either GNU libint or the Solaris gettext implementation.
Happy ;-).
Isn't it libintl, though?
> This new internationalization support is enabled by default. If
> gettext isn't available, or if Git is compiled with
> NO_GETTEXT=YesPlease, Git fall back on its current behavior of showing
s/fall/falls/;
> This change is somewhat large because as well as adding a C, Shell and
> Perl i18n interface we're adding a lot of tests for them, and for
> those tests to work we need a skeleton PO file to actually test
> translations.
You _could_ split it up this way, I think, if you really wanted to:
* The mechanism, i.e. integration with libintl and gettext, without any
translated strings but with the .pot file, with tests to make sure in a
locale that is missing *.mo files for it, we get the default output;
* Sample translation for is_IS locale, with tests to make sure that we
get translated output in a locale that has *.mo files for it.
But I am not sure if that is better.
> diff --git a/.gitignore b/.gitignore
> index 8572c8c..c47f3a8 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -224,3 +224,4 @@
> *.pdb
> /Debug/
> /Release/
> +/share/
I somehow find it distasteful that this is overly wide; same with the
change in Makefile to "rm -rf share/". I suspect it wouldn't be the case
if it were limited to share/locale/ or something.
But perhaps we would never ship anything in share/ and things in there
will always come from elsewhere.
> diff --git a/Makefile b/Makefile
> index ee34eab..896f5fd 100644
> --- a/Makefile
> +++ b/Makefile
> ...
> @@ -2435,6 +2507,7 @@ clean:
> $(RM) $(TEST_PROGRAMS)
> $(RM) -r bin-wrappers
> $(RM) -r $(dep_dirs)
> + $(RM) -r po/git.pot share/
It seems "distclean" tells us to clean po/git.pot, which hints at me that
normal "clean" shouldn't?
^ permalink raw reply
* Git 1.7.7.1 (Win7) work slower.
From: mardu @ 2011-11-15 7:59 UTC (permalink / raw)
To: git
Hello,
The latest Git version for Windows (1.7.7.1; GUI) work very slow (it means that I
have to wait tens seconds or even few minutes for open the application, scan
files, read history and other...).
I don't see changes in speed in another aplications installed on my computer.
The previous version Git (1.7.6), which I had installed, it has worked normaly.
OS:Windows 7 x64 (Home Premium, SP1),
CPU: Intel Core i5 M520 2.40GHz
RAM: 4GB
Martin
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2011, #03; Sun, 13)
From: Junio C Hamano @ 2011-11-15 7:27 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Vincent van Ravesteijn, Ramsay Jones, msysGit
In-Reply-To: <4EC0C101.4060001@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
> IMO, these two topics can move forward:
>
>> * vr/msvc (2011-10-31) 3 commits
>> * na/strtoimax (2011-11-05) 3 commits
Thanks.
^ permalink raw reply
* Re: [PATCH] Fix "is_refname_available(): query only possibly-conflicting references"
From: Junio C Hamano @ 2011-11-15 7:24 UTC (permalink / raw)
To: mhagger
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <1321336525-19374-1-git-send-email-mhagger@alum.mit.edu>
mhagger@alum.mit.edu writes:
> This patch can be squashed on top of "is_refname_available(): query
> only possibly-conflicting references", or applied at the end of
> mh/ref-api-take-2; it does not conflict with the two commits later in
> the series.
Thanks. At the microscopic level (i.e. in the context of the said series),
the patch makes sense to me.
However, I'd rather see us spend effort to make absolutely sure that other
topics already in next that touch the related codepaths (I think you have
two such series yourself and I suspect there are other minor fixes that
may textually conflict) are in good shape and have them graduate early
after 1.7.8 ships, before queuing a re-roll of the ref-api series, which
is rather extensive.
^ permalink raw reply
* Re: [PATCH, v2] tag: implement --[no-]strip option
From: Junio C Hamano @ 2011-11-15 7:17 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Kirill A. Shutemov, git
In-Reply-To: <4EC209C7.6090805@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> writes:
> Am 11/14/2011 22:43, schrieb Kirill A. Shutemov:
>> From: "Kirill A. Shutemov" <kirill@shutemov.name>
>>
>> --strip::
>> Remove from tag message lines staring with '#', trailing spaces
>
> s/staring/starting/
>
>> from every line and empty lines from the beginning and end.
>> Enabled by default. Use --no-strip to overwrite the behaviour.
>>
>> --no-strip is useful if you want to take a tag message as-is, without
>> any stripping.
>
> I would like to know why this is useful. Tag messages are for human
> consumption. What benefit is it that whitespace is not stripped? Why are
> lines starting with '#' so important that they need to stay in the tag
> message?
A conversion from foreign SCM comes to mind, but that is somewhat an
unfair comment, given that I know by heart how stripspace works and the
above documentation patch incorrectly describes what it does.
Besides stripping "# comment", stripspace collapses consecutive blank
lines, removes trailing blank lines and trailing whitespaces at the end of
lines.
^ permalink raw reply
* Re: [PATCH 01/10] Allow resolve_ref() caller to decide whether to receive static buffer
From: Junio C Hamano @ 2011-11-15 7:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: Jeff King, git
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> resolve_ref() is taught to return a xstrdup'd buffer if alloc is true.
> All callers are updated to receive static buffer as before though.
Thanks for working on this. I have a slight fear that this may collide
with some topics in flight, but that is something we can worry about after
1.7.8 release is done.
I like the direction of the entire series, and agree with the reasoning
behind the choices of callsites to be converted in later part of the
series (they are clearly documented).
After this series, all the callsites use either 0 or 1 and never a
computed value for the extra argument. I think it would make it a lot
safer and easier for other people who later want to touch the codepath
that has resolve_ref(..., 0) calls in it, if we instead did this in the
following way:
(1) Rename resolve_ref() to resolve_ref_unsafe() everywhere to make it
stand out in the code, without changing anything else. This is the
moral equivalent of a half or your [01/10].
(2) Introduce resolve_ref() that returns a copy. This is equivalent to
the other half of your [01/10].
(3) Convert the ones that should use resolve_ref() to do so. This is
equivalent to the remainder of your series.
(4) Convert callsites of resolve_ref_unsafe() that xstrdup()'s the
returned value back to use resolve_ref() as needed.
(5) And finally document in in-code comments at each tricky callsites of
resolve_ref_unsafe() why the use of the unsafe variant is OK in that
context. This can be done as part of step 1. We obviously do not have
to document trivial ones (e.g. a variable in a narrow sub-function
scope receives the return value of resolve_ref_unsafe(), and the
value is immediately used without giving it a longer lifetime).
The above is primarily a naming issue, but names are important. It forces
developers to _think_ before using something named _unsafe_, and helps
them to be on the lookout when they insert new code between an existing
call to the unsafe one and use of its result.
There are other "unsafe" functions like git_path() whose callsites can be
updated with the same strategy, and we probably should do that.
^ permalink raw reply
* Re: [PATCH, v2] tag: implement --[no-]strip option
From: Johannes Sixt @ 2011-11-15 6:42 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: git, Junio C Hamano
In-Reply-To: <1321307019-5557-1-git-send-email-kirill@shutemov.name>
Am 11/14/2011 22:43, schrieb Kirill A. Shutemov:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
>
> --strip::
> Remove from tag message lines staring with '#', trailing spaces
s/staring/starting/
> from every line and empty lines from the beginning and end.
> Enabled by default. Use --no-strip to overwrite the behaviour.
>
> --no-strip is useful if you want to take a tag message as-is, without
> any stripping.
I would like to know why this is useful. Tag messages are for human
consumption. What benefit is it that whitespace is not stripped? Why are
lines starting with '#' so important that they need to stay in the tag
message?
-- Hannes
^ permalink raw reply
* [PATCH 01/10] Allow resolve_ref() caller to decide whether to receive static buffer
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <20111115060603.GA17585@tre>
resolve_ref() is taught to return a xstrdup'd buffer if alloc is true.
All callers are updated to receive static buffer as before though.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
branch.c | 2 +-
builtin/branch.c | 6 +++---
builtin/checkout.c | 2 +-
builtin/commit.c | 2 +-
builtin/fmt-merge-msg.c | 2 +-
builtin/for-each-ref.c | 2 +-
builtin/fsck.c | 2 +-
builtin/merge.c | 2 +-
builtin/notes.c | 2 +-
builtin/receive-pack.c | 4 ++--
builtin/remote.c | 2 +-
builtin/show-branch.c | 4 ++--
builtin/symbolic-ref.c | 2 +-
cache.h | 2 +-
reflog-walk.c | 4 ++--
refs.c | 31 ++++++++++++++++++-------------
remote.c | 6 +++---
transport.c | 2 +-
wt-status.c | 2 +-
19 files changed, 43 insertions(+), 38 deletions(-)
diff --git a/branch.c b/branch.c
index d809876..a5ee614 100644
--- a/branch.c
+++ b/branch.c
@@ -151,7 +151,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
const char *head;
unsigned char sha1[20];
- head = resolve_ref("HEAD", sha1, 0, NULL);
+ head = resolve_ref("HEAD", sha1, 0, NULL, 0);
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
die("Cannot force update the current branch.");
}
diff --git a/builtin/branch.c b/builtin/branch.c
index 0fe9c4d..43b494c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -115,7 +115,7 @@ static int branch_merged(int kind, const char *name,
branch->merge[0] &&
branch->merge[0]->dst &&
(reference_name =
- resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
+ resolve_ref(branch->merge[0]->dst, sha1, 1, NULL, 0)) != NULL)
reference_rev = lookup_commit_reference(sha1);
}
if (!reference_rev)
@@ -250,7 +250,7 @@ static char *resolve_symref(const char *src, const char *prefix)
int flag;
const char *dst, *cp;
- dst = resolve_ref(src, sha1, 0, &flag);
+ dst = resolve_ref(src, sha1, 0, &flag, 0);
if (!(dst && (flag & REF_ISSYMREF)))
return NULL;
if (prefix && (cp = skip_prefix(dst, prefix)))
@@ -688,7 +688,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
track = git_branch_track;
- head = resolve_ref("HEAD", head_sha1, 0, NULL);
+ head = resolve_ref("HEAD", head_sha1, 0, NULL, 0);
if (!head)
die(_("Failed to resolve HEAD as a valid ref."));
head = xstrdup(head);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index beeaee4..f92c737 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -699,7 +699,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
unsigned char rev[20];
int flag;
memset(&old, 0, sizeof(old));
- old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag));
+ old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag, 0));
old.commit = lookup_commit_reference_gently(rev, 1);
if (!(flag & REF_ISSYMREF)) {
free((char *)old.path);
diff --git a/builtin/commit.c b/builtin/commit.c
index c46f2d1..11ae005 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
struct commit *commit;
struct strbuf format = STRBUF_INIT;
unsigned char junk_sha1[20];
- const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
+ const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL, 0);
struct pretty_print_context pctx = {0};
struct strbuf author_ident = STRBUF_INIT;
struct strbuf committer_ident = STRBUF_INIT;
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 7e2f225..c80f4b7 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -263,7 +263,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
const char *current_branch;
/* get current branch */
- current_branch = resolve_ref("HEAD", head_sha1, 1, NULL);
+ current_branch = resolve_ref("HEAD", head_sha1, 1, NULL, 0);
if (!current_branch)
die("No current branch");
if (!prefixcmp(current_branch, "refs/heads/"))
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index d90e5d2..09d7d6b 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -629,7 +629,7 @@ static void populate_value(struct refinfo *ref)
if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
unsigned char unused1[20];
const char *symref;
- symref = resolve_ref(ref->refname, unused1, 1, NULL);
+ symref = resolve_ref(ref->refname, unused1, 1, NULL, 0);
if (symref)
ref->symref = xstrdup(symref);
else
diff --git a/builtin/fsck.c b/builtin/fsck.c
index df1a88b..4d3d87e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -532,7 +532,7 @@ static int fsck_head_link(void)
if (verbose)
fprintf(stderr, "Checking HEAD link\n");
- head_points_at = resolve_ref("HEAD", head_sha1, 0, &flag);
+ head_points_at = resolve_ref("HEAD", head_sha1, 0, &flag, 0);
if (!head_points_at)
return error("Invalid HEAD");
if (!strcmp(head_points_at, "HEAD"))
diff --git a/builtin/merge.c b/builtin/merge.c
index 42b4f9e..d9c7a80 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1095,7 +1095,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* Check if we are _not_ on a detached HEAD, i.e. if there is a
* current branch.
*/
- branch = resolve_ref("HEAD", head_sha1, 0, &flag);
+ branch = resolve_ref("HEAD", head_sha1, 0, &flag, 0);
if (branch && !prefixcmp(branch, "refs/heads/"))
branch += 11;
if (!branch || is_null_sha1(head_sha1))
diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437d..9c70d8f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -825,7 +825,7 @@ static int merge_commit(struct notes_merge_options *o)
t = xcalloc(1, sizeof(struct notes_tree));
init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
- o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL);
+ o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL, 0);
if (!o->local_ref)
die("Failed to resolve NOTES_MERGE_REF");
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..903f2c5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -571,7 +571,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
int flag;
strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name);
- dst_name = resolve_ref(buf.buf, sha1, 0, &flag);
+ dst_name = resolve_ref(buf.buf, sha1, 0, &flag, 0);
strbuf_release(&buf);
if (!(flag & REF_ISSYMREF))
@@ -695,7 +695,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
check_aliased_updates(commands);
- head_name = resolve_ref("HEAD", sha1, 0, NULL);
+ head_name = resolve_ref("HEAD", sha1, 0, NULL, 0);
for (cmd = commands; cmd; cmd = cmd->next)
if (!cmd->skip_update)
diff --git a/builtin/remote.c b/builtin/remote.c
index 407abfb..4a3b529 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -573,7 +573,7 @@ static int read_remote_branches(const char *refname,
strbuf_addf(&buf, "refs/remotes/%s/", rename->old);
if (!prefixcmp(refname, buf.buf)) {
item = string_list_append(rename->remote_branches, xstrdup(refname));
- symref = resolve_ref(refname, orig_sha1, 1, &flag);
+ symref = resolve_ref(refname, orig_sha1, 1, &flag, 0);
if (flag & REF_ISSYMREF)
item->util = xstrdup(symref);
else
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 4b480d7..d8282ad 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -728,7 +728,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
static const char *fake_av[2];
const char *refname;
- refname = resolve_ref("HEAD", sha1, 1, NULL);
+ refname = resolve_ref("HEAD", sha1, 1, NULL, 0);
fake_av[0] = xstrdup(refname);
fake_av[1] = NULL;
av = fake_av;
@@ -791,7 +791,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
}
}
- head_p = resolve_ref("HEAD", head_sha1, 1, NULL);
+ head_p = resolve_ref("HEAD", head_sha1, 1, NULL, 0);
if (head_p) {
head_len = strlen(head_p);
memcpy(head, head_p, head_len + 1);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index dea849c..50cbdd1 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -12,7 +12,7 @@ static void check_symref(const char *HEAD, int quiet)
{
unsigned char sha1[20];
int flag;
- const char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag);
+ const char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag, 0);
if (!refs_heads_master)
die("No such ref: %s", HEAD);
diff --git a/cache.h b/cache.h
index 5badece..b2f0d56 100644
--- a/cache.h
+++ b/cache.h
@@ -866,7 +866,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);
*
* errno is sometimes set on errors, but not always.
*/
-extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);
+extern char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag, int alloc);
extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
diff --git a/reflog-walk.c b/reflog-walk.c
index 5d81d39..f71cca6 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,7 +50,7 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
for_each_reflog_ent(ref, read_one_reflog, reflogs);
if (reflogs->nr == 0) {
unsigned char sha1[20];
- const char *name = resolve_ref(ref, sha1, 1, NULL);
+ const char *name = resolve_ref(ref, sha1, 1, NULL, 0);
if (name)
for_each_reflog_ent(name, read_one_reflog, reflogs);
}
@@ -168,7 +168,7 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
else {
if (*branch == '\0') {
unsigned char sha1[20];
- const char *head = resolve_ref("HEAD", sha1, 0, NULL);
+ const char *head = resolve_ref("HEAD", sha1, 0, NULL, 0);
if (!head)
die ("No current branch");
free(branch);
diff --git a/refs.c b/refs.c
index 44c1c86..0387490 100644
--- a/refs.c
+++ b/refs.c
@@ -361,7 +361,7 @@ static int warn_if_dangling_symref(const char *refname, const unsigned char *sha
if (!(flags & REF_ISSYMREF))
return 0;
- resolves_to = resolve_ref(refname, junk, 0, NULL);
+ resolves_to = resolve_ref(refname, junk, 0, NULL, 0);
if (!resolves_to || strcmp(resolves_to, d->refname))
return 0;
@@ -497,16 +497,19 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)
return -1;
}
-const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)
+char *resolve_ref(const char *ref_ro, unsigned char *sha1, int reading, int *flag, int alloc)
{
int depth = MAXDEPTH;
ssize_t len;
+ char *ref;
char buffer[256];
static char ref_buffer[256];
if (flag)
*flag = 0;
+ ref = strcpy(ref_buffer, ref_ro);
+
if (check_refname_format(ref, REFNAME_ALLOW_ONELEVEL))
return NULL;
@@ -531,14 +534,14 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
if (!get_packed_ref(ref, sha1)) {
if (flag)
*flag |= REF_ISPACKED;
- return ref;
+ goto done;
}
/* The reference is not a packed reference, either. */
if (reading) {
return NULL;
} else {
hashclr(sha1);
- return ref;
+ goto done;
}
}
@@ -602,7 +605,9 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
*flag |= REF_ISBROKEN;
return NULL;
}
- return ref;
+
+done:
+ return alloc ? xstrdup(ref) : ref;
}
/* The argument to filter_refs */
@@ -614,7 +619,7 @@ struct ref_filter {
int read_ref_full(const char *ref, unsigned char *sha1, int reading, int *flags)
{
- if (resolve_ref(ref, sha1, reading, flags))
+ if (resolve_ref(ref, sha1, reading, flags, 0))
return 0;
return -1;
}
@@ -1118,7 +1123,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
this_result = refs_found ? sha1_from_ref : sha1;
mksnpath(fullref, sizeof(fullref), *p, len, str);
- r = resolve_ref(fullref, this_result, 1, &flag);
+ r = resolve_ref(fullref, this_result, 1, &flag, 0);
if (r) {
if (!refs_found++)
*ref = xstrdup(r);
@@ -1148,7 +1153,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
const char *ref, *it;
mksnpath(path, sizeof(path), *p, len, str);
- ref = resolve_ref(path, hash, 1, NULL);
+ ref = resolve_ref(path, hash, 1, NULL, 0);
if (!ref)
continue;
if (!stat(git_path("logs/%s", path), &st) &&
@@ -1184,7 +1189,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
lock = xcalloc(1, sizeof(struct ref_lock));
lock->lock_fd = -1;
- ref = resolve_ref(ref, lock->old_sha1, mustexist, &type);
+ ref = resolve_ref(ref, lock->old_sha1, mustexist, &type, 0);
if (!ref && errno == EISDIR) {
/* we are trying to lock foo but we used to
* have foo/bar which now does not exist;
@@ -1197,7 +1202,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
error("there are still refs under '%s'", orig_ref);
goto error_return;
}
- ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);
+ ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type, 0);
}
if (type_p)
*type_p = type;
@@ -1360,7 +1365,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldref);
- symref = resolve_ref(oldref, orig_sha1, 1, &flag);
+ symref = resolve_ref(oldref, orig_sha1, 1, &flag, 0);
if (flag & REF_ISSYMREF)
return error("refname %s is a symbolic ref, renaming it is not supported",
oldref);
@@ -1649,7 +1654,7 @@ int write_ref_sha1(struct ref_lock *lock,
unsigned char head_sha1[20];
int head_flag;
const char *head_ref;
- head_ref = resolve_ref("HEAD", head_sha1, 1, &head_flag);
+ head_ref = resolve_ref("HEAD", head_sha1, 1, &head_flag, 0);
if (head_ref && (head_flag & REF_ISSYMREF) &&
!strcmp(head_ref, lock->ref_name))
log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
@@ -1986,7 +1991,7 @@ int update_ref(const char *action, const char *refname,
int ref_exists(const char *refname)
{
unsigned char sha1[20];
- return !!resolve_ref(refname, sha1, 1, NULL);
+ return !!resolve_ref(refname, sha1, 1, NULL, 0);
}
struct ref *find_ref_by_name(const struct ref *list, const char *name)
diff --git a/remote.c b/remote.c
index 6655bb0..e776ba3 100644
--- a/remote.c
+++ b/remote.c
@@ -482,7 +482,7 @@ static void read_config(void)
return;
default_remote_name = xstrdup("origin");
current_branch = NULL;
- head_ref = resolve_ref("HEAD", sha1, 0, &flag);
+ head_ref = resolve_ref("HEAD", sha1, 0, &flag, 0);
if (head_ref && (flag & REF_ISSYMREF) &&
!prefixcmp(head_ref, "refs/heads/")) {
current_branch =
@@ -1007,7 +1007,7 @@ static char *guess_ref(const char *name, struct ref *peer)
struct strbuf buf = STRBUF_INIT;
unsigned char sha1[20];
- const char *r = resolve_ref(peer->name, sha1, 1, NULL);
+ const char *r = resolve_ref(peer->name, sha1, 1, NULL, 0);
if (!r)
return NULL;
@@ -1058,7 +1058,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
unsigned char sha1[20];
int flag;
- dst_value = resolve_ref(matched_src->name, sha1, 1, &flag);
+ dst_value = resolve_ref(matched_src->name, sha1, 1, &flag, 0);
if (!dst_value ||
((flag & REF_ISSYMREF) &&
prefixcmp(dst_value, "refs/heads/")))
diff --git a/transport.c b/transport.c
index 51814b5..063c285 100644
--- a/transport.c
+++ b/transport.c
@@ -163,7 +163,7 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
/* Follow symbolic refs (mainly for HEAD). */
localname = ref->peer_ref->name;
remotename = ref->name;
- tmp = resolve_ref(localname, sha, 1, &flag);
+ tmp = resolve_ref(localname, sha, 1, &flag, 0);
if (tmp && flag & REF_ISSYMREF &&
!prefixcmp(tmp, "refs/heads/"))
localname = tmp;
diff --git a/wt-status.c b/wt-status.c
index 70fdb76..96dc603 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -119,7 +119,7 @@ void wt_status_prepare(struct wt_status *s)
s->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
s->use_color = -1;
s->relative_paths = 1;
- head = resolve_ref("HEAD", sha1, 0, NULL);
+ head = resolve_ref("HEAD", sha1, 0, NULL, 0);
s->branch = head ? xstrdup(head) : NULL;
s->reference = "HEAD";
s->fp = stdout;
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 10/10] branch: request resolve_ref() to allocate new buffer
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/branch.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 43b494c..7a31b1e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -103,7 +103,7 @@ static int branch_merged(int kind, const char *name,
* safely to HEAD (or the other branch).
*/
struct commit *reference_rev = NULL;
- const char *reference_name = NULL;
+ char *reference_name = NULL;
int merged;
if (kind == REF_LOCAL_BRANCH) {
@@ -115,7 +115,7 @@ static int branch_merged(int kind, const char *name,
branch->merge[0] &&
branch->merge[0]->dst &&
(reference_name =
- resolve_ref(branch->merge[0]->dst, sha1, 1, NULL, 0)) != NULL)
+ resolve_ref(branch->merge[0]->dst, sha1, 1, NULL, 1)) != NULL)
reference_rev = lookup_commit_reference(sha1);
}
if (!reference_rev)
@@ -141,6 +141,7 @@ static int branch_merged(int kind, const char *name,
" '%s', even though it is merged to HEAD."),
name, reference_name);
}
+ free(reference_name);
return merged;
}
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 09/10] fmt-merge-msg: request resolve_ref() to allocate new buffer
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/fmt-merge-msg.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index c80f4b7..3d001d4 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -261,9 +261,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
int i = 0, pos = 0;
unsigned char head_sha1[20];
const char *current_branch;
+ char *ref;
/* get current branch */
- current_branch = resolve_ref("HEAD", head_sha1, 1, NULL, 0);
+ current_branch = ref = resolve_ref("HEAD", head_sha1, 1, NULL, 1);
if (!current_branch)
die("No current branch");
if (!prefixcmp(current_branch, "refs/heads/"))
@@ -283,8 +284,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
die ("Error in line %d: %.*s", i, len, p);
}
- if (!srcs.nr)
+ if (!srcs.nr) {
+ free(ref);
return 0;
+ }
if (merge_title)
do_fmt_merge_msg_title(out, current_branch);
@@ -306,6 +309,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
shortlog(origins.items[i].string, origins.items[i].util,
head, &rev, shortlog_len, out);
}
+ free(ref);
return 0;
}
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 08/10] notes: request resolve_ref() to allocate new buffer
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/notes.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index 9c70d8f..80c6e09 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -804,6 +804,8 @@ static int merge_commit(struct notes_merge_options *o)
struct notes_tree *t;
struct commit *partial;
struct pretty_print_context pretty_ctx;
+ char *ref;
+ int ret;
/*
* Read partial merge result from .git/NOTES_MERGE_PARTIAL,
@@ -825,7 +827,7 @@ static int merge_commit(struct notes_merge_options *o)
t = xcalloc(1, sizeof(struct notes_tree));
init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
- o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL, 0);
+ o->local_ref = ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL, 1);
if (!o->local_ref)
die("Failed to resolve NOTES_MERGE_REF");
@@ -843,7 +845,9 @@ static int merge_commit(struct notes_merge_options *o)
free_notes(t);
strbuf_release(&msg);
- return merge_abort(o);
+ ret = merge_abort(o);
+ free(ref);
+ return ret;
}
static int merge(int argc, const char **argv, const char *prefix)
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 07/10] receive-pack: request resolve_ref() to allocate new buffer
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/receive-pack.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 903f2c5..e447adb 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -36,7 +36,7 @@ static int use_sideband;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
static int auto_gc = 1;
-static const char *head_name;
+static char *head_name;
static int sent_capabilities;
static enum deny_action parse_deny_action(const char *var, const char *value)
@@ -695,7 +695,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
check_aliased_updates(commands);
- head_name = resolve_ref("HEAD", sha1, 0, NULL, 0);
+ free(head_name);
+ head_name = resolve_ref("HEAD", sha1, 0, NULL, 1);
for (cmd = commands; cmd; cmd = cmd->next)
if (!cmd->skip_update)
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 02/10] cmd_merge: convert to single exit point
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
This makes post-processing easier.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/merge.c | 48 +++++++++++++++++++++++++++++-------------------
1 files changed, 29 insertions(+), 19 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index d9c7a80..1be6f3b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1082,7 +1082,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
struct commit *head_commit;
struct strbuf buf = STRBUF_INIT;
const char *head_arg;
- int flag, i;
+ int flag, i, ret = 0;
int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
struct commit_list *common = NULL;
const char *best_strategy = NULL, *wt_strategy = NULL;
@@ -1121,7 +1121,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
die(_("There is no merge to abort (MERGE_HEAD missing)."));
/* Invoke 'git reset --merge' */
- return cmd_reset(nargc, nargv, prefix);
+ ret = cmd_reset(nargc, nargv, prefix);
+ goto done;
}
if (read_cache_unmerged())
@@ -1205,7 +1206,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
read_empty(remote_head->sha1, 0);
update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
DIE_ON_ERR);
- return 0;
+ goto done;
} else {
struct strbuf merge_names = STRBUF_INIT;
@@ -1292,7 +1293,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* but first the most common case of merging one remote.
*/
finish_up_to_date("Already up-to-date.");
- return 0;
+ goto done;
} else if (allow_fast_forward && !remoteheads->next &&
!common->next &&
!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
@@ -1313,15 +1314,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
strbuf_addstr(&msg,
" (no commit created; -m option ignored)");
o = want_commit(sha1_to_hex(remoteheads->item->object.sha1));
- if (!o)
- return 1;
-
- if (checkout_fast_forward(head_commit->object.sha1, remoteheads->item->object.sha1))
- return 1;
+ if (!o ||
+ checkout_fast_forward(head_commit->object.sha1,
+ remoteheads->item->object.sha1)) {
+ ret = 1;
+ goto done;
+ }
finish(head_commit, o->sha1, msg.buf);
drop_save();
- return 0;
+ goto done;
} else if (!remoteheads->next && common->next)
;
/*
@@ -1339,8 +1341,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
git_committer_info(IDENT_ERROR_ON_NO_NAME);
printf(_("Trying really trivial in-index merge...\n"));
if (!read_tree_trivial(common->item->object.sha1,
- head_commit->object.sha1, remoteheads->item->object.sha1))
- return merge_trivial(head_commit);
+ head_commit->object.sha1,
+ remoteheads->item->object.sha1)) {
+ ret = merge_trivial(head_commit);
+ goto done;
+ }
printf(_("Nope.\n"));
}
} else {
@@ -1368,7 +1373,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
}
if (up_to_date) {
finish_up_to_date("Already up-to-date. Yeeah!");
- return 0;
+ goto done;
}
}
@@ -1450,9 +1455,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* If we have a resulting tree, that means the strategy module
* auto resolved the merge cleanly.
*/
- if (automerge_was_ok)
- return finish_automerge(head_commit, common, result_tree,
- wt_strategy);
+ if (automerge_was_ok) {
+ ret = finish_automerge(head_commit, common, result_tree,
+ wt_strategy);
+ goto done;
+ }
/*
* Pick the result from the best strategy and have the user fix
@@ -1466,7 +1473,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
else
fprintf(stderr, _("Merge with strategy %s failed.\n"),
use_strategies[0]->name);
- return 2;
+ ret = 2;
+ goto done;
} else if (best_strategy == wt_strategy)
; /* We already have its result in the working tree. */
else {
@@ -1485,7 +1493,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (merge_was_ok) {
fprintf(stderr, _("Automatic merge went well; "
"stopped before committing as requested\n"));
- return 0;
} else
- return suggest_conflicts(option_renormalize);
+ ret = suggest_conflicts(option_renormalize);
+
+done:
+ return ret;
}
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 06/10] reflog-walk.c: request allocated buffer from resolve_ref()
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
for_each_reflog_ent() can do anything, including calling resolve_ref()
again. Therefore it's not safe to use the static buffer returned by
resolve_ref(). Request it to allocate new buffer instead.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
reflog-walk.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/reflog-walk.c b/reflog-walk.c
index f71cca6..00bdff8 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,9 +50,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
for_each_reflog_ent(ref, read_one_reflog, reflogs);
if (reflogs->nr == 0) {
unsigned char sha1[20];
- const char *name = resolve_ref(ref, sha1, 1, NULL, 0);
- if (name)
+ char *name = resolve_ref(ref, sha1, 1, NULL, 1);
+ if (name) {
for_each_reflog_ent(name, read_one_reflog, reflogs);
+ free(name);
+ }
}
if (reflogs->nr == 0) {
int len = strlen(ref);
--
1.7.4.74.g639db
^ 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