* Re: ANNOUNCE: Git for Windows 1.7.8
From: Sebastian Schuberth @ 2011-12-07 9:44 UTC (permalink / raw)
To: Pat Thoyts; +Cc: msysGit, Git Mailing List
In-Reply-To: <CABNJ2GJ_1Nf9rWev6BKE9zcqt5yrgq6PbaMLVRD705UapLHf0w@mail.gmail.com>
On 06.12.2011 21:32, Pat Thoyts wrote:
> This release brings the latest release of Git to Windows users.
>
> Pre-built installers are available from
> http://code.google.com/p/msysgit/downloads/list
Thanks a lot, Pat, for making the release!
--
Sebastian Schuberth
^ permalink raw reply
* Re: Auto update submodules after merge and reset
From: Andreas T.Auer @ 2011-12-07 9:07 UTC (permalink / raw)
To: git
In-Reply-To: <4ED5E9D2.4060503@web.de>
Jens Lehmann wrote:
> Am 30.11.2011 01:55, schrieb Max Krasnyansky:
> I'm working on a patch series to teach Git to optionally update the
> submodules work trees on checkout, reset merge and so on, but I'm not
> there yet.
>
>> I'm thinking about adding a config option that would enable automatic
>> submodule update but wanted to see if there is some fundamental reason
>> why it would not be accepted.
Because there is no good way to do so. It would be fine when you just track
the submodules "read-only", but if you are actually working on submodules,
it is a bad idea to always get a detached HEAD. It is also a bad idea to
merge or rebase on the currently checkedout branch. Because if you are
working on a maint branch in the submodule and then you checkout a pu branch
in the superproject, because you have forgotten that maint branch in the
submodule then all the proposed updates go to the maintenance branch -> bad.
So auto-update is not easy. But below I describe an idea that might solve
these issues and help auto-udpate to work in a sane way.
> I think adding something like an "submodule.autoupdate" config makes lots
> of sense, but IMO it should affect all work tree updating porcelain
> commands, not just merge.
I was thinking about submodule integration and had the idea to bind a
submodule to the superproject by having special references in the submodule
like refs/super/master, refs/super/featureX... So these references are like
tracking branches for the refs/heads/* of the superproject.
If you have tracking branches, the supermodule can just update the
corresponding branch. If this branch is currently checkedout and the work
area is clean, then the work area is updated, too. If there is currently a
local branch or a diffent super-branch checked out then the working area
should be considered "detached" from the superproject and not updated.
With this concept you could even switch branches in the superproject and the
attached submodules follow - still having no detached HEAD. When you want to
do some local work on the submodule you checkout a local branch and merge
back into the super branch later. The head of that super branch might have
changed by the update procedure meanwhile, but that is fine, then you just
have a merge instead of a fast-forward.
Another nice feature would be a recursive commit. So all changed index files
in the _attached_ submodules would first be committed in their submodules
and then the superproject commits too - all with one command. Currently it
feels a little bit like CVS - commit one file(submodule), commit the other
file(submodule) and then apply a label(commit the superproject) to keep the
changes together.
If the submodule is not attached the commit in the superproject can still
detect changes that have been made to the corresponding tracking branch and
pick these up.
As a summary: Tracking submodule branches in the superproject instead of
only the current HEAD of the submodule gives you more freedom to install
sane auto-update procedures. Even though it will raise a lot of detailed
questions like "should the refs/super/* be pushed/pulled when syncing the
submodule repositories".
^ permalink raw reply
* Re: [PATCH 2/2] run-command: Add interpreter permissions check
From: Frans Klaver @ 2011-12-07 8:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk469e2rn.fsf@alter.siamese.dyndns.org>
On Tue, Dec 6, 2011 at 11:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> If a script is started and the interpreter of that script given in the
>> shebang cannot be started due to permissions, we can get a rather
>> obscure situation. All permission checks pass for the script itself,
>> but we still get EACCES from execvp.
>>
>> Try to find out if the above is the case and warn the user about it.
>>
>> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> ---
>> run-command.c | 66 +++++++++++++++++++++++++++++++++++++++++++----
>> t/t0061-run-command.sh | 22 ++++++++++++++++
>> 2 files changed, 82 insertions(+), 6 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 5e38c5a..b8cf8d4 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
>> return 0;
>> }
>>
>> +static void check_interpreter(const char *cmd)
>> +{
>> + FILE *f;
>> + struct strbuf sb = STRBUF_INIT;
>> + /* bash reads an 80 character line when determining the interpreter.
>> + * BSD apparently only allows 32 characters, as it is the size of
>> + * your average binary executable header.
>> + */
>> + char firstline[80];
>> + char *interpreter = NULL;
>> + size_t s, i;
>> +
>> + f = fopen(cmd, "r");
>> + if (!f) {
>> + error("cannot open file '%s': %s\n", cmd, strerror(errno));
>> + return;
>> + }
>> +
>> + s = fread(firstline, 1, sizeof(firstline), f);
>> + if (s < 2) {
>> + trace_printf("cannot determine file type");
>> + fclose(f);
>> + return;
>> + }
>> +
>> + if (firstline[0] != '#' || firstline[1] != '!') {
>> + trace_printf("file '%s' is not a script or"
>> + " is a script without '#!'", cmd);
>> + fclose(f);
>> + return;
>> + }
>
> Nice touches to silently pass scripts that do not begin with she-bang.
>
>> +
>> + /* see if the given path has the executable bit set */
>> + for (i = 2; i < s; i++) {
>> + if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
>> + interpreter = firstline + i;
>> +
>> + if (interpreter && (firstline[i] == ' ' ||
>> + firstline[i] == '\n')) {
>
> Curious.
>
> "#!<TAB>/bin/bash<TAB><LF>" would cause you to check "/bin/bash<TAB>"?
Apparently so. Thanks for catching.
>> + strbuf_add(&sb, interpreter,
>> + (firstline + i) - interpreter);
>> + break;
>> + }
>
> Wouldn't strcspn() work better instead of this loop?
Probably. Will revise.
>> + }
>> + if (!sb.len) {
>> + error("could not determine interpreter");
>> + strbuf_release(&sb);
>> + return;
>> + }
>> +
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("bad interpreter: no read/execute permissions on '%s'\n",
>> + sb.buf);
>> +
>> + strbuf_release(&sb);
>> +}
>> +
>> static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> {
>> /* man 2 execve states that EACCES is returned for:
>> @@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> char *next;
>>
>> if (strchr(cmd, '/')) {
>> - if (!have_read_execute_permissions(cmd))
>> - error("no read/execute permissions on '%s'\n", cmd);
>> + if (have_read_execute_permissions(cmd))
>> + check_interpreter(cmd);
>
> I would have expected the overall logic to be more like this:
>
> if we cannot read and execute it then
> that in itself is an error (i.e. the error message from [1/2])
> else if we can read it then
> let's see if there is an error in the interpreter.
>
> It is unnatural to see "if we can read and execute, then see if there is
> anything wrong with the interpreter" and _nothing else_ here. If you made
> the "have_read_execute_permissions()" to issue the error message you used
> to give in your [1/2] patch here, that is OK from the point of view of the
> overall code structure, but then the function is no longer "do we have
> permissions" boolean check and needs to be renamed. And if you didn't,
> then I have to wonder why we do not need the error message you added in
> your [1/2].
Hm, yea makes sense. I'll rethink this a bit.
Again, thanks for the review.
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-07 8:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vpqg1e3au.fsf@alter.siamese.dyndns.org>
Thanks for the review. There's a lot of things you mention that I
either didn't see (staring blind, you know) or that I didn't know of.
On Tue, Dec 6, 2011 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> +#ifndef WIN32
>> +static int is_in_group(gid_t gid)
>> ...
>> +static int have_read_execute_permissions(const char *path)
>> +{
>> + struct stat s;
>> + trace_printf("checking '%s'\n", path);
>> +
>> + if (stat(path, &s) < 0) {
>> + ...
>> + /* check world permissions */
>> + if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
>> + return 1;
>
> Hmm, do you need to do this with stat(2)?
>
> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
> this much trouble?
Probably. I'll use access instead in a reroll.
> I also think that your permission check is incorrectly implemented.
>
> $ cd /var/tmp && date >j && chmod 044 j && ls -l j
> ----r--r-- 1 junio junio 29 Dec 6 14:32 j
> $ cat j
> cat: j: Permission denied
> $ su pogo
> Password:
> $ cat j
> Tue Dec 6 14:32:23 PST 2011
>
> That's a world-readable but unreadable-only-to-me file.
Hmm, this is a case that didn't fit my expectations. Thanks for catching.
>> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> +{
>> + /* man 2 execve states that EACCES is returned for:
>
> /*
> * Just a style, but we tend to write multi-line comment like
> * this, without anything else on opening and closing lines of
> * the comment block.
> */
>
>> + * - The file system is mounted noexec
>> + */
>> + struct strbuf sb = STRBUF_INIT;
>> + char *path = getenv("PATH");
>> + char *next;
>> +
>> + if (strchr(cmd, '/')) {
>> + if (!have_read_execute_permissions(cmd))
>> + error("no read/execute permissions on '%s'\n", cmd);
>> + return;
>> + }
>
> Ok, execvp() failed and "cmd" has at least one slash, so we know we did
> not look for it in $PATH. We check only one and return (did you need
> getenv() in that case?).
Obviously not. Missed that.
>
>> + for (;;) {
>> + next = strchrnul(path, ':');
>> + if (path < next)
>> + strbuf_add(&sb, path, next - path);
>> + else
>> + strbuf_addch(&sb, '.');
>
> Nice touch that you did not forget an empty component on $PATH.
Yes, that's a relic from me starting work based on one of your
proposed patches[1]. So that one goes to you.
[1] http://article.gmane.org/gmane.comp.version-control.git/171838
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("no read/execute permissions on '%s'\n", sb.buf);
>
> Don't you want to continue here upon error, after resetting sb? You just
> saw the directory is unreadble, so you know next file_exists() will fail
> before you try it.
Yes. I thought about that. I didn't do that because of the fact that I
had to do more than just resetting sb. The path variable has to be
updated as well. I had the choice of adding a level of indentation {},
duplicating the code, or just do a check I know before will fail.
There's probably something to say for each one of them. I'll probably
refactor that a bit more.
>> + if (sb.len && sb.buf[sb.len - 1] != '/')
>> + strbuf_addch(&sb, '/');
>> + strbuf_addstr(&sb, cmd);
>> +
>> + if (file_exists(sb.buf)) {
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("no read/execute permissions on '%s'\n",
>> + sb.buf);
>> + else
>> + warn("file '%s' exists and permissions "
>> + "seem OK.\nIf this is a script, see if you "
>> + "have sufficient privileges to run the "
>> + "interpreter", sb.buf);
>
> Does "warn()" do the right thing for multi-line strings like this?
I don't know/remember. It seemed like a natural thing to do, but I'll find out.
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Jonathan Nieder @ 2011-12-07 8:28 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207081734.GG11737@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Fine. But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
^in
> time is well spent.
Sorry for the nonsense. :) And now that I look back over previous
revisions of the patches, I see that I _hadn't_ clearly complained
about the same aspects of these particular commit messages before.
So what am I talking about here?
I guess it is just a pattern: commit messages I have looked at in the
sequencer series lately seem to focus too much on implementation
details and not enough on the "big picture" of what the user or
internal API user will experience. One symptom is that I get lost in
reading the commit message without looking at the patch. Another
symptom is that the commit messages tend to mention the particular
(private) functions that were changed, instead of the more prominent
(often public) callers that the reader might have cause to call.
Hoping that clarifies a little,
Jonathan
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Ramkumar Ramachandra @ 2011-12-07 8:26 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111207081734.GG11737@elie.hsd1.il.comcast.net>
Hi Jonathan,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
>> ($gname/186365) implemented.
>
> Thanks for this. I am worried that some of my comments in previous
> reviews (especially about change descriptions) were not addressed,
> which could mean one of a few things:
>
> - you didn't notice them
> - they were wrong
> - you noticed them and they were describing real problems, but it
> wasn't worth the time to fix them
>
> Fine. But I would like to know which case they fell into, so I can
> learn in order to do a better job reviewing the future and know my
> time is well spent.
I suppose they were a combination of all three. I'm sorry- I'll try
to be more careful and communicative in the future.
Thanks for this round of reviews!
-- Ram
^ permalink raw reply
* Re: Re: Git Submodule Problem - Bug?
From: Manuel Koller @ 2011-12-07 8:21 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Jens Lehmann, Fredrik Gustafsson, Thomas Rast, git
In-Reply-To: <20111129220303.GA2812@sandbox-rc.fritz.box>
> How about this:
>
> The user issues 'git submodule add foo' and we discover that there is
> already a local clone under the name foo. Git then asks something like
> this
>
> Error when adding: There is already a local submodule under the
> name 'foo'.
>
> You can either rename the submodule to be added to a different
> name or manually remove the local clone underneath
> .git/modules/foo. If you want to remove the local clone please
> quit now.
>
> We strongly suggest that you give each submodule a unique name.
> Note: This name is independent from the path it is bound to.
>
> What do you want me to do ([r]ename it, [Q]uit) ?
>
> When the user chooses 'rename' git will prompt for a new name.
>
> If we are going to support the remove use case with add we additionally
> need some logic to deal with it during update (which is not supported
> yet AFAIK). But we probably need this support anyway since between
> removal and adding a new submodule under the same can be a long time.
> If users switch between such ancient history and the new history we
> would have the same conflict.
>
> We could of course just error out and tell the user that he has to give
> the submodule an uniqe name. If the user does not do so leave it to him
> to deal with the situation manually.
>
> What do you think?
>
> Cheers Heiko
Prompt to choose another name would be fine I guess - but it solves
the problem only if the submodule has been initialized already. There
could be a submodule of the same name in another branch, which I
haven't checked out yet, for example. The user would have to be forced
choose a unique name for every submodule.
Anyway, it seems impossible to handle a name clash automatically,
since there are good reasons to have different urls for the same
submodule. Having read the thread linked by Junio, the only way out
seems to be a kind of url rewrite scheme and using the url as name.
Doesn't it solve all the problems?
- the url is more or less unique (there are problems now if we have to
different submodules at the same path, which is much more likely to
happen than a different repository at the same url some time in the
future)
- after a change of the submodule's url, we can still check out old
commits in a comfortable way
- we could have the same submodule at different paths, but downloaded only once
- the user is not forced to do anything, but the .gitmodule config can
still be overruled if necessary
^ permalink raw reply
* Re: [PATCH 0/5] Re-roll rr/revert-cherry-pick
From: Jonathan Nieder @ 2011-12-07 8:17 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> This is a re-roll of rr/revert-cherry-pick, with Junio's suggestions
> ($gname/186365) implemented.
Thanks for this. I am worried that some of my comments in previous
reviews (especially about change descriptions) were not addressed,
which could mean one of a few things:
- you didn't notice them
- they were wrong
- you noticed them and they were describing real problems, but it
wasn't worth the time to fix them
Fine. But I would like to know which case they fell into, so I can
learn in order to do a better job reviewing the future and know my
time is well spent.
The series makes various changes, all essentially good, which leaves
me all the more motivated to figure out how to get this sort of thing
in smoothly without making life difficult for people in the future
tracking down regressions.
Ciao,
Jonathan
^ permalink raw reply
* Re: [PATCH 4/2] grep: turn off threading for non-worktree
From: Thomas Rast @ 2011-12-07 8:12 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9ED1.8010502@lsrfire.ath.cx>
René Scharfe wrote:
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 47ac188..e981a9b 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -228,8 +228,9 @@ OPTIONS
> there is a match and with non-zero status when there isn't.
>
> --threads <n>::
> + Run <n> search threads in parallel. Default is 8 when searching
> + the worktree and 0 otherwise. This option is ignored if git was
> + built without support for POSIX threads.
[...]
> - nr_threads = (online_cpus() > 1) ? THREADS : 0;
> + nr_threads = (online_cpus() > 1 && !list.nr) ? THREADS : 0;
It would be more consistent to stick to the pack.threads convention
where 0 means "all of my cores", so to disable threading the user
would set the number of threads to 1. Or were you trying to measure
the contention between the worker thread and the add_work() thread?
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH v2 3/3] grep: disable threading in all but worktree case
From: Thomas Rast @ 2011-12-07 8:11 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Eric Herman, Junio C Hamano
In-Reply-To: <4EDE9BBA.2010409@lsrfire.ath.cx>
René Scharfe wrote:
> Am 02.12.2011 17:15, schrieb René Scharfe:
> > How about adding a parameter to control the number of threads
> > (--threads?) instead that defaults to eight (or five) for the worktree
> > and one for the rest? That would also make benchmarking easier.
>
> Like this:
>
> -- >8 --
> Subject: grep: add parameter --threads
>
> Allow the number of threads to be specified by the user. This makes
> benchmarking the performance impact of different numbers of threads
> much easier.
Sounds good, though in the end we would also want to have a config
variable for the poor OS X users who have to tune their threads
*down*... :-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 5/5] revert: simplify communicating command-line arguments
From: Jonathan Nieder @ 2011-12-07 8:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-6-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> From: Jonathan Nieder <jrnieder@gmail.com>
>
> Currently, command-line arguments are communicated using (argc, argv)
> until a prepare_revs() turns it into a terse structure. However,
> since we plan to expose the cherry-picking machinery through a public
> API in the future, we want callers to be able to call in with a
> filled-in structure. For the revert builtin, this means that the
> chief argument parser, parse_args(), should parse into such a
> structure. Make this change.
Fine. I guess it can be said more clearly while emphasizing public
interfaces over implementation details:
Since the "multiple cherry-pick" feature was introduced (commit
7e2bfd3f, 2010-07-02), the pick/revert machinery has kept track
of the set of commits to be cherry-picked or reverted using
commit_argc and commit_argv variables storing the relevant
command-line parameters.
Future callers as other commands are built in (am, rebase,
sequencer) may find it easier to pass rev-list options to this
machinery in already-parsed form, though. So teach
cmd_cherry_pick and cmd_revert to parse the rev-list arguments
in advance and pass the commit set to pick_revisions() as a
value of type "struct rev_info".
[...]
> @@ -222,7 +220,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> "-x", opts->record_origin,
> "--edit", opts->edit,
> NULL);
> - opts->commit_argv = argv;
> +
> + if (opts->subcommand == REPLAY_NONE) {
> + opts->revs = xmalloc(sizeof(*opts->revs));
> + init_revisions(opts->revs, NULL);
> + opts->revs->no_walk = 1;
> + if (argc < 2)
> + usage_with_options(usage_str, options);
> + argc = setup_revisions(argc, argv, opts->revs, NULL);
> + } else
> + opts->revs = NULL;
> +
> + /* Forbid stray command-line arguments */
> + if (argc > 1)
> + usage_with_options(usage_str, options);
> }
Looks good.
Tests? What happens if I try "git cherry-pick --all"? How about "git
cherry-pick HEAD..HEAD"?
FWIW, with the commit message clarified and with or without new tests,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH] userdiff: allow * between cpp funcname words
From: Thomas Rast @ 2011-12-07 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vfwgxflkv.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
>
> > Actually (sadly) I'll have to revise it. It doesn't match much of C++
> > either, and I haven't yet come up with a reasonable regex that
> > matches, say,
> >
> > foo::Bar<int>::t& Baz::operator<<(
> >
> > which I would call ludicrous, but it's valid C++.
>
> Heh, I'd rather not see us go that route, which would either end up
> implementing a C++ parser or reverting the heuristics back to "non-blank
> at the beginning of the line" that was already reasonably useful.
Well, there are many things that we deliberately do not match right
now and for which that's a good thing:
label:
public:
void declaration_only(...);
int global_variable;
At some point I was wondering whether it would be better to just
declare a non-match for '.*;' and '^[A-Za-z_][A-Za-z_0-9]+:', and
otherwise match all '^[A-Za-z].*\(' but I may be missing something.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 5/5] reset: update cache-tree data when appropriate
From: Thomas Rast @ 2011-12-07 7:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Carlos Martín Nieto
In-Reply-To: <7v62hte1k7.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> > @@ -84,6 +85,12 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
> > return error(_("Failed to find tree of %s."), sha1_to_hex(sha1));
> > if (unpack_trees(nr, desc, &opts))
> > return -1;
> > +
> > + if (reset_type == MIXED || reset_type == HARD) {
> > + tree = parse_tree_indirect(sha1);
> > + prime_cache_tree(&active_cache_tree, tree);
> > + }
>
> The basic idea that MIXED or HARD should result in a cache-tree that match
> the tree we just read is sound, but how expensive is prime_cache_tree()? I
> think it reads the same tree once again. Admittedly, the data needed to
> reconstruct the tree is likely to be hot in core, but it may be necessary
> to measure before deciding if this is a good change.
Oh, I just didn't bother with timings because it's the same strategy
that read-tree follows, so I assumed if it was worth it back then, it
should again be a win.
For linux-2.6 as usual and best-of-10:
git reset before: 0:00.25real 0.12user 0.11system
after: 0:00.28real 0.14user 0.12system
git reset --hard before: 0:00.21real 0.13user 0.06system
after: 0:00.18real 0.10user 0.07system
So we spend ~30ms of extra user time, at a possible gain of 30% in
future git-commit invocations, as mentioned in the cover letter. I
think that's worth it :-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 4/5] revert: allow mixed pick and revert instructions
From: Jonathan Nieder @ 2011-12-07 7:45 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Parse the instruction sheet in '.git/sequencer/todo' as a list of
> (action, operand) pairs, instead of assuming that all instructions use
> the same action. Now you can do:
>
> pick fdc0b12 picked
> revert 965fed4 anotherpick
>
> For cherry-pick and revert, this means that a 'git cherry-pick
> --continue' can continue an ongoing revert operation and viceversa.
Good idea.
> This patch lays the foundation for extending the parser to support
> more actions so 'git rebase -i' can reuse this machinery in the
> future. While at it, also improve the error messages reported by the
> insn sheet parser.
I don't know what this part is about...
[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -39,7 +39,6 @@ static const char * const cherry_pick_usage[] = {
> NULL
> };
>
> -enum replay_action { REVERT, CHERRY_PICK };
Removing the enum, to expose it in sequencer.h. What does this have
to do with the stated goal of the patch?
> enum replay_subcommand {
> REPLAY_NONE,
> REPLAY_REMOVE_STATE,
> @@ -73,14 +72,14 @@ struct replay_opts {
>
> static const char *action_name(const struct replay_opts *opts)
> {
> - return opts->action == REVERT ? "revert" : "cherry-pick";
> + return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
[... lots of similar changes snipped ...]
> @@ -467,7 +466,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
> return run_command_v_opt(args, RUN_GIT_CMD);
> }
>
> -static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> +static int do_pick_commit(struct commit *commit, enum replay_action action,
> + struct replay_opts *opts)
Ok, now we're at the title feature.
Passing "action" as a separate parameter in addition to "opts" makes
sense since it can vary from item to item on the todo list. It
unfortunately makes reviewers' lives hard since it is easy to miss
additional unconverted uses of opts->action in the function and
compilers will not warn about that.
[...]
> if (index_differs_from("HEAD", 0))
> return error_dirty_index(opts);
[...]
> if (parent && parse_commit(parent) < 0)
> /* TRANSLATORS: The first %s will be "revert" or
> "cherry-pick", the second %s a SHA1 */
> return error(_("%s: cannot parse parent commit %s"),
> action_name(opts), sha1_to_hex(parent->object.sha1));
[...]
> res = do_recursive_merge(base, next, base_label, next_label,
> head, &msgbuf, opts);
Makes sense. This is the command name passed on the command line, not
the action name from the todo list, so it is left alone.
[...]
> @@ -607,13 +607,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
[...]
> - error(opts->action == REVERT
> + error(action == REPLAY_REVERT
> ? _("could not revert %s... %s")
> : _("could not apply %s... %s"),
... and that's the last use of "action" in this function on the
current master branch. Good.
[... more distracting s/REVERT/REPLAY_REVERT/ stuff snipped...]
> @@ -683,49 +683,55 @@ static void read_and_refresh_cache(struct replay_opts *opts)
[...]
> -static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
> - struct replay_opts *opts)
> +static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
Looks good.
[...]
> -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
> +static int parse_insn_line(char *bol, char *eol,
> + struct replay_insn_list *item, int lineno)
[...]
> - } else
> - return NULL;
> + } else {
> + size_t len = strchrnul(bol, '\n') - bol;
> + if (len > 255)
> + len = 255;
> + return error(_("Line %d: Unrecognized action: %.*s"),
> + lineno, (int)len, bol);
> + }
A new error message when an insn line does not start with "pick" or
"revert". Looks like a good change, but what does it have to do with
the subject of the patch?
[...]
> @@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
[...]
> status = get_sha1(bol, commit_sha1);
> *end_of_object_name = saved;
[...]
> if (status < 0)
> - return NULL;
> + return error(_("Line %d: Malformed object name: %s"), lineno, bol);
(Not about this patch, but an earlier one) What is this idiom about?
Why not
if (get_sha1(bol, commit_sha1))
return error(_(...));
?
[...]
> -static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
> - struct replay_opts *opts)
> +static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
Nice to see.
> {
> - struct commit_list **next = todo_list;
> - struct commit *commit;
> + struct replay_insn_list **next = todo_list;
> + struct replay_insn_list item = {0, NULL, NULL};
Style: elsewhere in this file, initializers look like "{ 0, NULL, NULL }"
(with space between the braces and values).
[...]
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
Thanks for writing these.
> test_line_count = 4 commits
> '
>
> +test_expect_success 'revert --continue continues after cherry-pick' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick base..anotherpick &&
Stops in "picked", asking user to resolve conflicts.
> + echo "c" >foo &&
> + git add foo &&
> + git commit &&
User resolves conflicts, as in the previous test.
> + git revert --continue &&
And asks to continue.
> + test_path_is_missing .git/sequencer &&
> + {
> + git rev-list HEAD |
> + git diff-tree --root --stdin |
> + sed "s/$_x40/OBJID/g"
> + } >actual &&
> + cat >expect <<-\EOF &&
> + OBJID
> + :100644 100644 OBJID OBJID M foo
> + OBJID
> + :100644 100644 OBJID OBJID M foo
> + OBJID
> + :100644 100644 OBJID OBJID M unrelated
> + OBJID
> + :000000 100644 OBJID OBJID A foo
> + :000000 100644 OBJID OBJID A unrelated
> + EOF
> + test_cmp expect actual
I wonder if there is some simpler way to check the result of resuming
a cherry-pick sequence, though that doesn't have much to do with the
patch. I guess I'd find
echo d >expect &&
... &&
# index is clean
git update-index --refresh &&
git rev-list HEAD >commits &&
test_line_count = 4 commits &&
test_cmp expect foo &&
test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
a more convincing demonstration that the cherry-pick finished without
incident.
> +'
> +
> +test_expect_success 'mixed pick and revert instructions' '
> + pristine_detach initial &&
> + test_must_fail git cherry-pick base..anotherpick &&
> + echo "c" >foo &&
> + git add foo &&
> + git commit &&
Same setup.
> + oldsha=`git rev-parse --short HEAD~1` &&
> + echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
Adding to the insn sheet. Makes sense.
Summing up: I quite like the idea of this patch, but I would be a lot
happier if changes unrelated to its goal were split off to be
considered separately.
> Acked-by: Jonathan Nieder <jrnider@gmail.com>
No, not acked.
Thanks,
Jonathan
^ permalink raw reply
* Re: How to make devs write better commit messages
From: Thomas Koch @ 2011-12-07 7:06 UTC (permalink / raw)
To: Joseph Huttner; +Cc: git
In-Reply-To: <CAOJsP-X0ZWT5HLHcBc2FmhoMpWFOvEFADiM9jGZ9R1ctqHDJ9w@mail.gmail.com>
Joseph Huttner:
> The bottom line is that good commit messages are really important, so
> we should make it as easy as possible for developers to go ahead and
> write a perfect commit message every time they commit code.
I recently started to work with the code review system Gerrit[1]. First I did
not pay attention to it, but later I was amazed that the commit message can
(and should) be reviewed just like the code changes.
So if you're using a code review prozess (you should!) then also include the
commit message in the review.
[1] http://en.wikipedia.org/wiki/Gerrit_%28software%29
Thomas Koch, http://www.koch.ro
^ permalink raw reply
* Re: [bug?] checkout -m doesn't work without a base version
From: Pete Harlan @ 2011-12-07 7:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbormn8vk.fsf@alter.siamese.dyndns.org>
On 12/05/2011 10:58 AM, Junio C Hamano wrote:
> Pete Harlan <pgit@pcharlan.com> writes:
>
>> But this only works if there's a base version; if foo.c was added in
>> each branch, we get:
>>
>> error: path 'foo.c' does not have all three versions
>>
>> Git didn't need all three versions to create the original conflicted
>> file, so why would it need them to recreate it?
>
> Because the original "merge" was a bit more carefully written but
> "checkout -m" was written without worrying too much about "both sides
> added differently" corner case and still being defensive about not doing
> random thing upon getting an unexpected input state.
>
> IOW, being lazy ;-)
>
> How does this look?
Wow, thanks for the quick response! That does indeed let me checkout
the file as expected.
I wrote a test (below) to be folded in with your patch, but the test
fails because it expects the restored file to be the same as the
originally-conflicted file, but the conflict-line labels change from
"HEAD" and "master":
<<<<<<< HEAD
in_topic
=======
in_master
>>>>>>> master
to "ours" and "theirs". (The same thing happens in the 3-way merge
case.)
If the label change is expected then I can rewrite the test to ignore
labels, or to expect "ours" and "theirs", whichever you think is best.
If the label change is unexpected, then I guess the test is good :)
--Pete
-- >8 --
Test 'checkout -m -- path' when path is a 2-way merge
Signed-off-by: Pete Harlan <pgit@pcharlan.com>
---
t/t2023-checkout-m-twoway.sh | 29 +++++++++++++++++++++++++++++
1 files changed, 29 insertions(+), 0 deletions(-)
create mode 100755 t/t2023-checkout-m-twoway.sh
diff --git a/t/t2023-checkout-m-twoway.sh b/t/t2023-checkout-m-twoway.sh
new file mode 100755
index 0000000..5b50360
--- /dev/null
+++ b/t/t2023-checkout-m-twoway.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='checkout -m -- conflicted/path/with/2-way/merge
+
+Ensures that checkout -m on a resolved file restores the conflicted file
+when it conflicted with a 2-way merge.'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ test_tick &&
+ test_commit initial_commit &&
+ git branch topic &&
+ test_commit added_in_master file.txt in_master &&
+ git checkout topic &&
+ test_commit added_in_topic file.txt in_topic
+'
+
+test_must_fail git merge master
+
+test_expect_success '-m restores 2-way conflicted+resolved file' '
+ cp file.txt file.txt.conflicted &&
+ echo resolved >file.txt &&
+ git add file.txt &&
+ git checkout -m -- file.txt &&
+ test_cmp file.txt.conflicted file.txt
+'
+
+test_done
^ permalink raw reply related
* Re: [PATCH 3/5] revert: simplify getting commit subject in format_todo()
From: Jonathan Nieder @ 2011-12-07 7:06 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> format_todo() calls get_message(), but uses only the subject line of
> the commit message. Save work and unnecessary memory allocations by
> using find_commit_subject() instead. Also, remove the spurious check
> on cur->item: the previous patch makes sure that instruction sheets
> with missing commit subjects are parsable.
So, what effect will this patch actually have? Is it an optimization
with no other observable effect?
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Jonathan Nieder @ 2011-12-07 7:02 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-3-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Change the instruction sheet format subtly so that the subject of the
> commit message that follows the object name is optional. As a result,
> an instruction sheet like this is now perfectly valid:
>
> pick 35b0426
> pick fbd5bbcbc2e
> pick 7362160f
>
> While at it, also fix a bug: currently, we use a commit-id-shaped
> buffer to store the word after "pick" in '.git/sequencer/todo'. This
> is both wasteful and wrong because it places an artificial limit on
> the line length. Eliminate the need for the buffer altogether, and
> add a test demonstrating this.
>
> [jc: simplify parsing]
Reading the above does not make it at all obvious that I should want
to apply this patch because otherwise my prankster friend can cause my
copy of git to crash or run arbitrary code by putting a long commit
name in .git/sequencer/todo in our NFS-mounted shared checkout.
(Yes, I know there are other problems with such a setup, especially if
.git/hooks or .git/config is writable by untrusted people. So it is
not actually a security bug, but a robustness one.)
^ permalink raw reply
* Re: [PATCH 1/5] revert: free msg in format_todo()
From: Jonathan Nieder @ 2011-12-07 6:43 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List
In-Reply-To: <1323239877-24101-2-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Memory allocated to the fields of msg by get_message() isn't freed.
> This is potentially a big leak,
[because it is in a loop over commits being picked]
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
> builtin/revert.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1ea525c..0c6d3d8 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
> if (get_message(cur->item, &msg))
> return error(_("Cannot get commit message for %s"), sha1_abbrev);
> strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
> + free_message(&msg);
> }
> return 0;
> }
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-07 6:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7h29fkfy.fsf@alter.siamese.dyndns.org>
On Tue, Dec 06, 2011 at 01:40:17PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > ... You can now
> > do: "git credential-store erase </dev/null" to erase everything
> > (since you have provided no restrictions, it matches everything).
>
> That "justification" does not sound so true to me but perhaps that is
> because it is unclear what "erase" means and what it means to give the
> operation parameters.
It's not meant to be a justification, but rather an explanation. I think
the behavior is probably too dangerous to leave.
> When I see "erase $foo", I would find it natural if $foo meant "if there
> is something that matches $foo, then please remove it, but keep everything
> else intact", and not the other way around "Match the existing entries
> against a pattern (or a set of matching patterns) I am giving you, and
> drop all the rest". So if I happen to give you an empty set, I would
> expect nothing is removed.
It does do the first thing you mentioned (you provide one pattern $foo,
and we match the pattern you have given). It's just that the pattern you
have specified is "everything". The problem is not in the matching, but
in the pattern specification language.
This pattern:
protocol=https
host=github.com
means "match everything that uses https _and_ has a host of github.com".
The username and path fields are not present, which implicitly means
"don't care about them".
Similarly, this pattern:
protocol=https
means "match everything that uses https". Everything else is not
specified, and therefore we allow anything.
Then what does the empty pattern do? It cares about nothing, and
therefore matches everything.
By itself, I don't think that is a problem. It's something you might
want to specify, and it's logically consistent with the way the patterns
are matched. What is dangerous, though, is that failing to provide
input is byte-wise identical to the empty pattern. And that's why I say
it's a pattern specification problem.
A rough BNF for the pattern format is something like:
pattern = *line
line = key "=" value
key = *<any byte except NUL, "=", or "\n">
value = *<any byte except NUL or "\n">
Because the pattern takes 0 or more lines and no terminator, we can't
distinguish between empty or truncated input and the empty pattern. So
one solution would be:
pattern = *line "\n"
i.e., require a blank line terminator.
Does that explain the issue better?
-Peff
^ permalink raw reply
* [PATCH 5/5] revert: simplify communicating command-line arguments
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
From: Jonathan Nieder <jrnieder@gmail.com>
Currently, command-line arguments are communicated using (argc, argv)
until a prepare_revs() turns it into a terse structure. However,
since we plan to expose the cherry-picking machinery through a public
API in the future, we want callers to be able to call in with a
filled-in structure. For the revert builtin, this means that the
chief argument parser, parse_args(), should parse into such a
structure. Make this change.
[rr: minor improvements, commit message]
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 53 +++++++++++++++++++++++++++++------------------------
1 files changed, 29 insertions(+), 24 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index f2bf198..3b25a0c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -59,13 +59,14 @@ struct replay_opts {
int allow_rerere_auto;
int mainline;
- int commit_argc;
- const char **commit_argv;
/* Merge strategy */
const char *strategy;
const char **xopts;
size_t xopts_nr, xopts_alloc;
+
+ /* Only used by REPLAY_NONE */
+ struct rev_info *revs;
};
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -168,9 +169,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
die(_("program error"));
}
- opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
- PARSE_OPT_KEEP_ARGV0 |
- PARSE_OPT_KEEP_UNKNOWN);
+ argc = parse_options(argc, argv, NULL, options, usage_str,
+ PARSE_OPT_KEEP_ARGV0 |
+ PARSE_OPT_KEEP_UNKNOWN);
/* Check for incompatible subcommands */
verify_opt_mutually_compatible(me,
@@ -212,9 +213,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
NULL);
}
- else if (opts->commit_argc < 2)
- usage_with_options(usage_str, options);
-
if (opts->allow_ff)
verify_opt_compatible(me, "--ff",
"--signoff", opts->signoff,
@@ -222,7 +220,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
"-x", opts->record_origin,
"--edit", opts->edit,
NULL);
- opts->commit_argv = argv;
+
+ if (opts->subcommand == REPLAY_NONE) {
+ opts->revs = xmalloc(sizeof(*opts->revs));
+ init_revisions(opts->revs, NULL);
+ opts->revs->no_walk = 1;
+ if (argc < 2)
+ usage_with_options(usage_str, options);
+ argc = setup_revisions(argc, argv, opts->revs, NULL);
+ } else
+ opts->revs = NULL;
+
+ /* Forbid stray command-line arguments */
+ if (argc > 1)
+ usage_with_options(usage_str, options);
}
struct commit_message {
@@ -631,23 +642,15 @@ static int do_pick_commit(struct commit *commit, enum replay_action action,
return res;
}
-static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
+static void prepare_revs(struct replay_opts *opts)
{
- int argc;
-
- init_revisions(revs, NULL);
- revs->no_walk = 1;
if (opts->action != REPLAY_REVERT)
- revs->reverse = 1;
+ opts->revs->reverse ^= 1;
- argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
- if (argc > 1)
- usage(*revert_or_cherry_pick_usage(opts));
-
- if (prepare_revision_walk(revs))
+ if (prepare_revision_walk(opts->revs))
die(_("revision walk setup failed"));
- if (!revs->commits)
+ if (!opts->revs->commits)
die(_("empty commit set passed"));
}
@@ -837,14 +840,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
struct replay_opts *opts)
{
- struct rev_info revs;
struct commit *commit;
struct replay_insn_list **next;
- prepare_revs(&revs, opts);
+ prepare_revs(opts);
next = todo_list;
- while ((commit = get_revision(&revs)))
+ while ((commit = get_revision(opts->revs)))
next = replay_insn_list_append(opts->action, commit, next);
}
@@ -1037,6 +1039,9 @@ static int pick_revisions(struct replay_opts *opts)
struct replay_insn_list *todo_list = NULL;
unsigned char sha1[20];
+ if (opts->subcommand == REPLAY_NONE)
+ assert(opts->revs);
+
read_and_refresh_cache(opts);
/*
--
1.7.7.3
^ permalink raw reply related
* [PATCH 4/5] revert: allow mixed pick and revert instructions
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
Parse the instruction sheet in '.git/sequencer/todo' as a list of
(action, operand) pairs, instead of assuming that all instructions use
the same action. Now you can do:
pick fdc0b12 picked
revert 965fed4 anotherpick
For cherry-pick and revert, this means that a 'git cherry-pick
--continue' can continue an ongoing revert operation and viceversa.
This patch lays the foundation for extending the parser to support
more actions so 'git rebase -i' can reuse this machinery in the
future. While at it, also improve the error messages reported by the
insn sheet parser.
Helped-by: Jonathan Nieder <jrnider@gmail.com>
Acked-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 136 +++++++++++++++++++--------------------
sequencer.h | 8 ++
t/t3510-cherry-pick-sequence.sh | 58 +++++++++++++++++
3 files changed, 133 insertions(+), 69 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 706b8d4..f2bf198 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,7 +39,6 @@ static const char * const cherry_pick_usage[] = {
NULL
};
-enum replay_action { REVERT, CHERRY_PICK };
enum replay_subcommand {
REPLAY_NONE,
REPLAY_REMOVE_STATE,
@@ -73,14 +72,14 @@ struct replay_opts {
static const char *action_name(const struct replay_opts *opts)
{
- return opts->action == REVERT ? "revert" : "cherry-pick";
+ return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
}
static char *get_encoding(const char *message);
static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
{
- return opts->action == REVERT ? revert_usage : cherry_pick_usage;
+ return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
}
static int option_parse_x(const struct option *opt,
@@ -159,7 +158,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
OPT_END(),
};
- if (opts->action == CHERRY_PICK) {
+ if (opts->action == REPLAY_PICK) {
struct option cp_extra[] = {
OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
@@ -363,7 +362,7 @@ static int error_dirty_index(struct replay_opts *opts)
return error_resolve_conflict(action_name(opts));
/* Different translation strings for cherry-pick and revert */
- if (opts->action == CHERRY_PICK)
+ if (opts->action == REPLAY_PICK)
error(_("Your local changes would be overwritten by cherry-pick."));
else
error(_("Your local changes would be overwritten by revert."));
@@ -467,7 +466,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
return run_command_v_opt(args, RUN_GIT_CMD);
}
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+ struct replay_opts *opts)
{
unsigned char head[20];
struct commit *base, *next, *parent;
@@ -542,7 +542,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
defmsg = git_pathdup("MERGE_MSG");
- if (opts->action == REVERT) {
+ if (action == REPLAY_REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -583,7 +583,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
}
- if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+ if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
head, &msgbuf, opts);
write_message(&msgbuf, defmsg);
@@ -607,13 +607,13 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
* However, if the merge did not even start, then we don't want to
* write it at all.
*/
- if (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
+ if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
- if (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))
+ if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
write_cherry_pick_head(commit, "REVERT_HEAD");
if (res) {
- error(opts->action == REVERT
+ error(action == REPLAY_REVERT
? _("could not revert %s... %s")
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -637,7 +637,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
init_revisions(revs, NULL);
revs->no_walk = 1;
- if (opts->action != REVERT)
+ if (opts->action != REPLAY_REVERT)
revs->reverse = 1;
argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
@@ -683,49 +683,55 @@ static void read_and_refresh_cache(struct replay_opts *opts)
* assert(commit_list_count(list) == 2);
* return list;
*/
-static struct commit_list **commit_list_append(struct commit *commit,
- struct commit_list **next)
+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+ struct commit *operand,
+ struct replay_insn_list **next)
{
- struct commit_list *new = xmalloc(sizeof(struct commit_list));
- new->item = commit;
+ struct replay_insn_list *new = xmalloc(sizeof(*new));
+ new->action = action;
+ new->operand = operand;
*next = new;
new->next = NULL;
return &new->next;
}
-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
- struct replay_opts *opts)
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
{
- struct commit_list *cur = NULL;
- const char *sha1_abbrev = NULL;
- const char *action_str = opts->action == REVERT ? "revert" : "pick";
- const char *subject;
- int subject_len;
+ struct replay_insn_list *cur;
for (cur = todo_list; cur; cur = cur->next) {
- sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
- subject_len = find_commit_subject(cur->item->buffer, &subject);
+ const char *sha1_abbrev, *action_str, *subject;
+ int subject_len;
+
+ action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+ sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+ subject_len = find_commit_subject(cur->operand->buffer, &subject);
strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
subject_len, subject);
}
return 0;
}
-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
+static int parse_insn_line(char *bol, char *eol,
+ struct replay_insn_list *item, int lineno)
{
unsigned char commit_sha1[20];
- enum replay_action action;
char *end_of_object_name;
int saved, status;
if (!prefixcmp(bol, "pick ")) {
- action = CHERRY_PICK;
+ item->action = REPLAY_PICK;
bol += strlen("pick ");
} else if (!prefixcmp(bol, "revert ")) {
- action = REVERT;
+ item->action = REPLAY_REVERT;
bol += strlen("revert ");
- } else
- return NULL;
+ } else {
+ size_t len = strchrnul(bol, '\n') - bol;
+ if (len > 255)
+ len = 255;
+ return error(_("Line %d: Unrecognized action: %.*s"),
+ lineno, (int)len, bol);
+ }
end_of_object_name = bol + strcspn(bol, " \n");
saved = *end_of_object_name;
@@ -733,37 +739,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
status = get_sha1(bol, commit_sha1);
*end_of_object_name = saved;
- /*
- * Verify that the action matches up with the one in
- * opts; we don't support arbitrary instructions
- */
- if (action != opts->action) {
- const char *action_str;
- action_str = action == REVERT ? "revert" : "cherry-pick";
- error(_("Cannot %s during a %s"), action_str, action_name(opts));
- return NULL;
- }
-
if (status < 0)
- return NULL;
+ return error(_("Line %d: Malformed object name: %s"), lineno, bol);
- return lookup_commit_reference(commit_sha1);
+ item->operand = lookup_commit_reference(commit_sha1);
+ if (!item->operand)
+ return error(_("Line %d: Not a valid commit: %s"), lineno, bol);
+
+ item->next = NULL;
+ return 0;
}
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
- struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
{
- struct commit_list **next = todo_list;
- struct commit *commit;
+ struct replay_insn_list **next = todo_list;
+ struct replay_insn_list item = {0, NULL, NULL};
char *p = buf;
int i;
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
- commit = parse_insn_line(p, eol, opts);
- if (!commit)
- return error(_("Could not parse line %d."), i);
- next = commit_list_append(commit, next);
+ if (parse_insn_line(p, eol, &item, i) < 0)
+ return -1;
+ next = replay_insn_list_append(item.action, item.operand, next);
p = *eol ? eol + 1 : eol;
}
if (!*todo_list)
@@ -771,8 +769,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
return 0;
}
-static void read_populate_todo(struct commit_list **todo_list,
- struct replay_opts *opts)
+static void read_populate_todo(struct replay_insn_list **todo_list)
{
const char *todo_file = git_path(SEQ_TODO_FILE);
struct strbuf buf = STRBUF_INIT;
@@ -788,7 +785,7 @@ static void read_populate_todo(struct commit_list **todo_list,
}
close(fd);
- res = parse_insn_buffer(buf.buf, todo_list, opts);
+ res = parse_insn_buffer(buf.buf, todo_list);
strbuf_release(&buf);
if (res)
die(_("Unusable instruction sheet: %s"), todo_file);
@@ -837,18 +834,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
die(_("Malformed options sheet: %s"), opts_file);
}
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
struct replay_opts *opts)
{
struct rev_info revs;
struct commit *commit;
- struct commit_list **next;
+ struct replay_insn_list **next;
prepare_revs(&revs, opts);
next = todo_list;
while ((commit = get_revision(&revs)))
- next = commit_list_append(commit, next);
+ next = replay_insn_list_append(opts->action, commit, next);
}
static int create_seq_dir(void)
@@ -946,7 +943,7 @@ fail:
return -1;
}
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static void save_todo(struct replay_insn_list *todo_list)
{
const char *todo_file = git_path(SEQ_TODO_FILE);
static struct lock_file todo_lock;
@@ -954,7 +951,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
int fd;
fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
- if (format_todo(&buf, todo_list, opts) < 0)
+ if (format_todo(&buf, todo_list) < 0)
die(_("Could not format %s."), todo_file);
if (write_in_full(fd, buf.buf, buf.len) < 0) {
strbuf_release(&buf);
@@ -998,9 +995,10 @@ static void save_opts(struct replay_opts *opts)
}
}
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct replay_insn_list *todo_list,
+ struct replay_opts *opts)
{
- struct commit_list *cur;
+ struct replay_insn_list *cur;
int res;
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -1010,8 +1008,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
read_and_refresh_cache(opts);
for (cur = todo_list; cur; cur = cur->next) {
- save_todo(cur, opts);
- res = do_pick_commit(cur->item, opts);
+ save_todo(cur);
+ res = do_pick_commit(cur->operand, cur->action, opts);
if (res) {
if (!cur->next)
/*
@@ -1036,7 +1034,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
static int pick_revisions(struct replay_opts *opts)
{
- struct commit_list *todo_list = NULL;
+ struct replay_insn_list *todo_list = NULL;
unsigned char sha1[20];
read_and_refresh_cache(opts);
@@ -1056,7 +1054,7 @@ static int pick_revisions(struct replay_opts *opts)
if (!file_exists(git_path(SEQ_TODO_FILE)))
return error(_("No %s in progress"), action_name(opts));
read_populate_opts(&opts);
- read_populate_todo(&todo_list, opts);
+ read_populate_todo(&todo_list);
/* Verify that the conflict has been resolved */
if (!index_differs_from("HEAD", 0))
@@ -1074,7 +1072,7 @@ static int pick_revisions(struct replay_opts *opts)
if (create_seq_dir() < 0)
return -1;
if (get_sha1("HEAD", sha1)) {
- if (opts->action == REVERT)
+ if (opts->action == REPLAY_REVERT)
return error(_("Can't revert as initial commit"));
return error(_("Can't cherry-pick into empty head"));
}
@@ -1091,7 +1089,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
memset(&opts, 0, sizeof(opts));
if (isatty(0))
opts.edit = 1;
- opts.action = REVERT;
+ opts.action = REPLAY_REVERT;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
res = pick_revisions(&opts);
@@ -1106,7 +1104,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
int res;
memset(&opts, 0, sizeof(opts));
- opts.action = CHERRY_PICK;
+ opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
res = pick_revisions(&opts);
diff --git a/sequencer.h b/sequencer.h
index f435fdb..71cfcae 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,6 +7,14 @@
#define SEQ_TODO_FILE "sequencer/todo"
#define SEQ_OPTS_FILE "sequencer/opts"
+enum replay_action { REPLAY_REVERT, REPLAY_PICK };
+
+struct replay_insn_list {
+ enum replay_action action;
+ struct commit *operand;
+ struct replay_insn_list *next;
+};
+
/*
* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
* any errors. Intended to be used by 'git reset'.
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 6390f2a..d52ad59 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -356,4 +356,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
test_line_count = 4 commits
'
+test_expect_success 'revert --continue continues after cherry-pick' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ git revert --continue &&
+ test_path_is_missing .git/sequencer &&
+ {
+ git rev-list HEAD |
+ git diff-tree --root --stdin |
+ sed "s/$_x40/OBJID/g"
+ } >actual &&
+ cat >expect <<-\EOF &&
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :000000 100644 OBJID OBJID A foo
+ :000000 100644 OBJID OBJID A unrelated
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'mixed pick and revert instructions' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ oldsha=`git rev-parse --short HEAD~1` &&
+ echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo &&
+ git cherry-pick --continue &&
+ test_path_is_missing .git/sequencer &&
+ {
+ git rev-list HEAD |
+ git diff-tree --root --stdin |
+ sed "s/$_x40/OBJID/g"
+ } >actual &&
+ cat >expect <<-\EOF &&
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :000000 100644 OBJID OBJID A foo
+ :000000 100644 OBJID OBJID A unrelated
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
1.7.7.3
^ permalink raw reply related
* [PATCH 3/5] revert: simplify getting commit subject in format_todo()
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
format_todo() calls get_message(), but uses only the subject line of
the commit message. Save work and unnecessary memory allocations by
using find_commit_subject() instead. Also, remove the spurious check
on cur->item: the previous patch makes sure that instruction sheets
with missing commit subjects are parsable.
Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 70055e5..706b8d4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -697,16 +697,16 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
struct replay_opts *opts)
{
struct commit_list *cur = NULL;
- struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
const char *sha1_abbrev = NULL;
const char *action_str = opts->action == REVERT ? "revert" : "pick";
+ const char *subject;
+ int subject_len;
for (cur = todo_list; cur; cur = cur->next) {
sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
- if (get_message(cur->item, &msg))
- return error(_("Cannot get commit message for %s"), sha1_abbrev);
- strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
- free_message(&msg);
+ subject_len = find_commit_subject(cur->item->buffer, &subject);
+ strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+ subject_len, subject);
}
return 0;
}
--
1.7.7.3
^ permalink raw reply related
* [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
Change the instruction sheet format subtly so that the subject of the
commit message that follows the object name is optional. As a result,
an instruction sheet like this is now perfectly valid:
pick 35b0426
pick fbd5bbcbc2e
pick 7362160f
While at it, also fix a bug: currently, we use a commit-id-shaped
buffer to store the word after "pick" in '.git/sequencer/todo'. This
is both wasteful and wrong because it places an artificial limit on
the line length. Eliminate the need for the buffer altogether, and
add a test demonstrating this.
[jc: simplify parsing]
Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 37 ++++++++++++++++---------------------
t/t3510-cherry-pick-sequence.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 21 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 0c6d3d8..70055e5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -711,31 +711,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
return 0;
}
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
{
unsigned char commit_sha1[20];
- char sha1_abbrev[40];
enum replay_action action;
- int insn_len = 0;
- char *p, *q;
+ char *end_of_object_name;
+ int saved, status;
- if (!prefixcmp(start, "pick ")) {
+ if (!prefixcmp(bol, "pick ")) {
action = CHERRY_PICK;
- insn_len = strlen("pick");
- p = start + insn_len + 1;
- } else if (!prefixcmp(start, "revert ")) {
+ bol += strlen("pick ");
+ } else if (!prefixcmp(bol, "revert ")) {
action = REVERT;
- insn_len = strlen("revert");
- p = start + insn_len + 1;
+ bol += strlen("revert ");
} else
return NULL;
- q = strchr(p, ' ');
- if (!q)
- return NULL;
- q++;
-
- strlcpy(sha1_abbrev, p, q - p);
+ end_of_object_name = bol + strcspn(bol, " \n");
+ saved = *end_of_object_name;
+ *end_of_object_name = '\0';
+ status = get_sha1(bol, commit_sha1);
+ *end_of_object_name = saved;
/*
* Verify that the action matches up with the one in
@@ -748,7 +744,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
return NULL;
}
- if (get_sha1(sha1_abbrev, commit_sha1) < 0)
+ if (status < 0)
return NULL;
return lookup_commit_reference(commit_sha1);
@@ -763,13 +759,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
int i;
for (i = 1; *p; i++) {
- commit = parse_insn_line(p, opts);
+ char *eol = strchrnul(p, '\n');
+ commit = parse_insn_line(p, eol, opts);
if (!commit)
return error(_("Could not parse line %d."), i);
next = commit_list_append(commit, next);
- p = strchrnul(p, '\n');
- if (*p)
- p++;
+ p = *eol ? eol + 1 : eol;
}
if (!*todo_list)
return error(_("No commits parsed."));
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 2c4c1c8..6390f2a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -13,6 +13,9 @@ test_description='Test cherry-pick continuation features
. ./test-lib.sh
+# Repeat first match 10 times
+_r10='\1\1\1\1\1\1\1\1\1\1'
+
pristine_detach () {
git cherry-pick --quit &&
git checkout -f "$1^0" &&
@@ -328,4 +331,29 @@ test_expect_success 'malformed instruction sheet 2' '
test_must_fail git cherry-pick --continue
'
+test_expect_success 'malformed instruction sheet 3' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "resolved" >foo &&
+ git add foo &&
+ git commit &&
+ sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet &&
+ cp new_sheet .git/sequencer/todo &&
+ test_must_fail git cherry-pick --continue
+'
+
+test_expect_success 'commit descriptions in insn sheet are optional' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ echo "c" >foo &&
+ git add foo &&
+ git commit &&
+ cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
+ cp new_sheet .git/sequencer/todo &&
+ git cherry-pick --continue &&
+ test_path_is_missing .git/sequencer &&
+ git rev-list HEAD >commits &&
+ test_line_count = 4 commits
+'
+
test_done
--
1.7.7.3
^ permalink raw reply related
* [PATCH 1/5] revert: free msg in format_todo()
From: Ramkumar Ramachandra @ 2011-12-07 6:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder
In-Reply-To: <1323239877-24101-1-git-send-email-artagnon@gmail.com>
Memory allocated to the fields of msg by get_message() isn't freed.
This is potentially a big leak, because fresh memory is allocated to
store the commit message for each commit. Fix this using
free_message().
Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/revert.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..0c6d3d8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -706,6 +706,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
if (get_message(cur->item, &msg))
return error(_("Cannot get commit message for %s"), sha1_abbrev);
strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+ free_message(&msg);
}
return 0;
}
--
1.7.7.3
^ 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