* Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork
From: Jeff King @ 2011-08-09 20:05 UTC (permalink / raw)
To: René Scharfe; +Cc: Johannes Sixt, Erik Faye-Lund, Junio C Hamano, git
In-Reply-To: <4E410B26.1080407@lsrfire.ath.cx>
On Tue, Aug 09, 2011 at 12:25:42PM +0200, René Scharfe wrote:
> > BTW, as nice as this "gzip -cn | cat" idea is, I think it needs to be
> > wrapped in a shell script. With the code above, we will generate "gzip
> > -cn | cat -9".
>
> Yes, the three added lines in the patch above would have to be moved
> down two lines, after the compression level is added. D'oh!
Also, is adding "| cat" also sufficient for arbitrary shell code (i.e.,
whatever the user hands us via the config)? I couldn't think of an
example that wouldn't work.
> OK, that's one way to do it; another would be let gzip (and bzip2 etc.)
> do whatever cat does to avoid end of line conversions. And yet another
> is to take them from http://unxutils.sourceforge.net/.
Yeah, I like all of those solutions better than hacking an extra pipe
into git. I don't know enough to say how painful they are in practice,
though.
> > BTW, from what Johannes said, the issue is about a non-msys program
> > calling an msys one. Does that mean that having git run:
> >
> > sh -c 'gzip -cn'
> >
> > would work? If so, then could the solution be as simple as turning off
> > the "don't bother with the shell" optimization that run-command uses?
> > Something like "gzip -cn" gets split by git and run via spawn now
> > (because it has no metacharacters). But we could easily make it always
> > run through the shell.
>
> Just checked -- it doesn't work. I assume that's because the shell is
> also an MSYS program.
Too bad. I guess the first msys program to be run sets the mode of the
output descriptor, and then everybody else inherits that. If there were
a helper or shell builtin to switch to binary mode, we could put it at
the start of the shell pipeline.
If you write a C program that does:
setmode(1, O_BINARY);
and call it like:
./setmode-helper; gzip -cn
does that work? I suspect not, as from my brief reading of the msys gzip
source, it actually calls setmode() itself.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] On Cygwin support both UNIX and DOS style path-names
From: Pascal Obry @ 2011-08-09 20:38 UTC (permalink / raw)
To: Ramsay Jones; +Cc: git
In-Reply-To: <4E416D4A.40602@ramsay1.demon.co.uk>
Le 09/08/2011 19:24, Ramsay Jones a écrit :
> Pascal Obry wrote:
>> In fact Cygwin supports both, so make Git agree with this.
>> The failing case is when a file is committed in a sub-dir of the
>> repository using a log message from a file specified with a DOS
>> style path-name. To reproduce:
>>
>> $ cd src
>> $ git commit -F c:\tmp\log.txt file.c
>> fatal: could not read log file 'src/c:\tmp\log.txt': No such file \
>> or directory.
>
> Hmm, are you using bash or cmd.exe? Using bash I get the following:
bash.
>
> $ cd src
> $ git commut -F c:\tmp\log.txt file.c
> fatal: could not read file 'src/c:tmplog.txt': No such file or directory
> $
>
> Which is what I would expect of (any) posix shell, viz:
>
> $ ls c:\
> > ^C
> $ ls c:\\
> AUTOEXEC.BAT* NTDETECT.COM* WATCOM/ msysgit/
> CMPNENTS/ Program Files/ WINDOWS/ msysgit-old/
> CONFIG.SYS* RECYCLER/ boot.ini* ntldr*
> Documents and Settings/ SUPPORT/ cygwin/ pagefile.sys
> I386/ SWSTAMP.TXT* cygwintemp/ ssl/
> IO.SYS* System Volume Information/ dm/ uname/
> MSDOS.SYS* TOOLSCD/ dm840/ zlib/
> MSOCache/ VALUEADD/ hiberfil.sys
Exactly, \\ this is what I have used and this is the bug. Cygwin
supports ls c:\\ so should Cygwin/Git. My quoted example was missing the
escape \.
--
--|------------------------------------------------------
--| Pascal Obry Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--| http://www.obry.net - http://v2p.fr.eu.org
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver keys.gnupg.net --recv-key F949BD3B
^ permalink raw reply
* Re: [PATCH] push: Don't push a repository with unpushed submodules
From: Junio C Hamano @ 2011-08-09 21:12 UTC (permalink / raw)
To: Fredrik Gustafsson; +Cc: git, hvoigt, jens.lehmann
In-Reply-To: <1312913752-21555-1-git-send-email-iveqy@iveqy.com>
Fredrik Gustafsson <iveqy@iveqy.com> writes:
> When working with submodules it is easy to forget to push a
> submodule to the server but pushing a super-project that
> contains a commit for that submodule. The result is that the
> superproject points at a submodule commit that is not available
> on the server.
>
> This adds the option --recurse-submodules=check to push. When
> using this option git will check that all submodule commits that
> are about to be pushed are present on a remote of the submodule.
Looking better.
> This does not guarantee that all submodules a super-project
> needs will be available on the server.
Hmm, why not?
You do not, when this option is in effect, push a new commit of the
superproject to the remote unless you know all the submodule commits
pointed at by the superproject commit you are pushing out are already
pushed out, right? If so how could some submodules become unavailable on
the other end?
Are you thinking about some submodules that you did not touch may have
been updated by somebody else, rewinding the branch that contained the
commit that you did not modify and that you have in your superproject
commit, or something? But ...
> In that case both the
> super-project and the submodules would need an atomic push.
... such a case won't be helped with any atomic push, as you didn't even
update that submodule that somebody else rewound.
So you must be thinking about something else. Can you clarify the failure
mode you are worried about a bit better here?
You may also drop the "atomic push might help something" from the
description; as it is unclear what it could help. Then what remains would
be...
> This
> does however prevent the human error of forgetting to push a
> submodule.
...this one, which is correct. This change is primarily to help prevent a
human error that was described in the first paragraph of the message (I do
not think it prevents any other kind of error). So while it is correct, it
may not have to be said again here.
> The first iteration of this patch series can be found here:
> http://thread.gmane.org/gmane.comp.version-control.git/176328/focus=176327
And the second iteration was here:
http://thread.gmane.org/gmane.comp.version-control.git/177992
> Documentation/git-push.txt | 6 ++
> builtin/push.c | 17 +++++
> submodule.c | 129 ++++++++++++++++++++++++++++++++++++++++
> submodule.h | 1 +
> t/t5531-deep-submodule-push.sh | 99 ++++++++++++++++++++++++++++++
> transport.c | 9 +++
> transport.h | 1 +
> 7 files changed, 262 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 88acfcd..6ae6ba3 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -162,6 +162,12 @@ useful if you write an alias or script around 'git push'.
> is specified. This flag forces progress status even if the
> standard error stream is not directed to a terminal.
>
> +--recurse-submodules=check::
> + Check whether all submodule commits used by the revisions to be
> + pushed are available on a remote tracking branch. Otherwise the
> + push will be aborted and the command will exit with non-zero status.
> +
> +
> include::urls-remotes.txt[]
>
> OUTPUT
> diff --git a/builtin/push.c b/builtin/push.c
> index 9cebf9e..23ba365 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -8,6 +8,7 @@
> #include "remote.h"
> #include "transport.h"
> #include "parse-options.h"
> +#include "submodule.h"
>
> static const char * const push_usage[] = {
> "git push [<options>] [<repository> [<refspec>...]]",
> @@ -219,6 +220,19 @@ static int do_push(const char *repo, int flags)
> return !!errs;
> }
>
> +static int option_parse_recurse_submodules(const struct option *opt,
> + const char *arg, int unset)
> +{
> + int *flags = opt->value;
> + if (arg) {
> + if (!strcmp(arg, "check"))
> + *flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
> + else
> + die("bad %s argument: %s", opt->long_name, arg);
> + }
> + return 0;
> +}
Later you may want to add a configuration variable to flip the default,
and _before_ that happens (e.g. like "now"), you would need to be able to
accept --recurse-submodules=no-check or something that defeats the
configured default from the command line during the transition period.
Why not use a simple string-valued variable to keep the last value of this
option you saw instead of using a callback? The overall control flow in
cmd_push() would look like this:
const char *submodules = NULL; /* default */
...
OPT_STRING(0, "recurse-submodules", &submodules, ...),
...
argc = parse_options(argc, argv, prefix, options, push_usage, 0);
parse_recurse_submodules_option(submodules, &flags);
and you set or unset the necessary bits in flags in the helper function.
That would also help people who use an alias with --recurse-submodules=
early on the command line to force his own default before configuration is
introduced by naturally implementing the usual "last one on the command
line wins" rule, no?
> int cmd_push(int argc, const char **argv, const char *prefix)
> {
> int flags = 0;
> @@ -236,6 +250,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
> OPT_BIT('n' , "dry-run", &flags, "dry run", TRANSPORT_PUSH_DRY_RUN),
> OPT_BIT( 0, "porcelain", &flags, "machine-readable output", TRANSPORT_PUSH_PORCELAIN),
> OPT_BIT('f', "force", &flags, "force updates", TRANSPORT_PUSH_FORCE),
> + { OPTION_CALLBACK, 0, "recurse-submodules", &flags, "check",
> + "controls recursive pushing of submodules",
> + PARSE_OPT_OPTARG, option_parse_recurse_submodules },
> OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
> OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
> OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
> diff --git a/submodule.c b/submodule.c
> index 1ba9646..c44472f 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -308,6 +308,135 @@ void set_config_fetch_recurse_submodules(int value)
> config_fetch_recurse_submodules = value;
> }
>
> +static int has_remote(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
> +{
> + return 1;
> +}
> +
> +/*
> + * checks whether a certain submodule commit is pushed based on the
> + * submodules remote tracking branches. returns 1 in case no remote
> + * tracking branches are found.
> + */
> +static int is_submodule_commit_pushed(const char *path, const unsigned char sha1[20])
> +{
> + int is_pushed = 0;
> + if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {
> + if (for_each_remote_ref_submodule(path, has_remote, NULL)) {
This code assumes that this for_each_remote_ref_submodule() call will
return non-zero only when has_remote() is called even once. Is that really
true? Doesn't the function (not has_remote() but the iterator) want to
signal errors in any way, and if so doesn't it return a negative value or
something?
> + struct child_process cp;
> + const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
> + struct strbuf buf = STRBUF_INIT;
> +
> + argv[1] = sha1_to_hex(sha1);
> + memset(&cp, 0, sizeof(cp));
> + cp.argv = argv;
> + cp.env = local_repo_env;
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.out = -1;
> + cp.dir = path;
> + if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))
> + is_pushed = 1;
> + close(cp.out);
> + strbuf_release(&buf);
Hmm, this run_command(), while it may not be a bug, feels somewhat fishy.
It calls start and then calls finish. What if the subprocess is really
fast, writes everything it needs to write (which is presumably 41 bytes at
most), closes its end of the pipe? And then you read from the other end of
the pipe. Am I being too paranoid to think that this sequence is ...
start_command();
strbuf_read();
finish_command();
close();
strbuf_release();
... a more natural way of writing this logic?
> + } else
> + is_pushed = 1;
This case is not "we know it is pushed" but "we do not know (and we do not
care) if it was pushed, because the submodule does not have a tracking",
no?
Probably the function should be named "submodule_needs_pushing()" and
return the value accordingly. Then "we do not have to prevent the user
from pushing the superproject based on the state of this submodule" case
can say "No, this submodule repository does not need pushing", which reads
a lot more naturally. The overall code structure would also become
simpler:
static int submodule_needs_pushing(...)
{
int needs_pushing = 0;
if (!(we can inspect the objects in this submodule))
return needs_pushing;
if (for_each_remote_ref_in_submodule(...)) {
... run rev-list ...
if (found commit not in remotes)
needs_pushing = 1;
... clean-up rev-list ...
}
return needs_pushing;
}
What happens in a submodule repository that you do not have a checkout?
the first if() statement in the function would be false, and you would end
up returning "is-pushed? no" and collect_unpushed_submodules_from_revs()
(which also is a misnomer; you do not care if commits in certain
submodules haven't been pushed---it should collect submodules that "needs
pushing", which is subtly different from "have not been pushed") that
called this function will say "Ah I found one, I found one, that is an
error!".
And I think that is wrong.
Imagine a merge in a superproject between two branches, among which only
one of them updated the submodule while the other one did not touch. As
far as that submodule path is concerned, the merge should resolve cleanly
without a checkout (or clone) of the submodule repository to take the
update from the branch that updated the submodule commit.
So the "submodule-needs-pushing()" function should return "no need to push
(there is nothing to push to begin with)" for a submodule that is not even
"submodule init"ed.
> +static int collect_unpushed_submodules_in_tree(const unsigned char *sha1,
> + const char *base, int baselen,
> + const char *pathname, unsigned mode,
> + int stage, void *context)
> +{
> + int *found_unpushed_submodules = context;
> + struct strbuf path = STRBUF_INIT;
> +
> + strbuf_add(&path, base, strlen(base));
> + strbuf_add(&path, pathname, strlen(pathname));
> +
> + if (S_ISGITLINK(mode) && !is_submodule_commit_pushed(path.buf, sha1)) {
> + *found_unpushed_submodules = 1;
> + return 0;
> + }
> + return READ_TREE_RECURSIVE;
> +}
I find it somewhat distasteful to have this separate codepath (and
tree_commits_pushed() function) only for the root commit. Why not use the
same diff-tree machinery?
> +static void parent_commits_pushed(struct commit *commit, struct commit_list *parent, int *found_unpushed_submodule)
> +{
> + while (parent) {
Also I wonder if you really want to do pair-wise diff here, which would
mean that for a typical two-parent merge you will potentially have to
check the same submodule twice (no I am not talking about the cost of
running diff the same number of times as you have parents, but I am
talking about the cost of running submodule_needs_pushing()).
Would it make sense to run the equivalent of "show -c --name-only" here
internally ("diff-tree --root -c --name-only" to be more exact)? Then any
submodule path that match one of the parents you would not have to check
at all. Just thinking aloud...
> + struct diff_options diff_opts;
> + diff_setup(&diff_opts);
> + DIFF_OPT_SET(&diff_opts, RECURSIVE);
> + diff_opts.output_format |= DIFF_FORMAT_CALLBACK;
> + diff_opts.format_callback = collect_unpushed_submodules_from_revs;
> + diff_opts.format_callback_data = found_unpushed_submodule;
> + if (diff_setup_done(&diff_opts) < 0)
> + die("diff_setup_done failed");
> + diff_tree_sha1(parent->item->object.sha1, commit->object.sha1, "", &diff_opts);
> + diffcore_std(&diff_opts);
> + diff_flush(&diff_opts);
> + parent = parent->next;
> + }
> +}
> +
> +static void tree_commits_pushed(struct commit *commit, int *found_unpushed_submodule)
> +{
> + struct tree * tree;
(style) struct tree *tree;
But I already suggested to get rid of this separate codepath, so it is not
such a big deal.
> + struct pathspec pathspec;
> + tree = parse_tree_indirect(commit->object.sha1);
> + init_pathspec(&pathspec,NULL);
> + read_tree_recursive(tree, "", 0, 0, &pathspec, collect_unpushed_submodules_in_tree,
> + found_unpushed_submodule);
> +}
> +
> +int check_for_unpushed_submodule_commits(unsigned char new_sha1[20])
> +{
The same "we are not looking for 'unpushed' ones, but for 'needs pushing'
ones" comment on naming applies here.
> + struct rev_info rev;
> + struct commit *commit;
> + const char *argv[] = {NULL, NULL, "--not", "--remotes", NULL};
> + int argc = ARRAY_SIZE(argv) - 1;
> + char *sha1_copy;
> + int found_unpushed_submodule = 0;
> +
> + init_revisions(&rev, NULL);
> + sha1_copy = xstrdup(sha1_to_hex(new_sha1));
> + argv[1] = sha1_copy;
> + setup_revisions(argc, argv, &rev, NULL);
> + if (prepare_revision_walk(&rev))
> + die("revision walk setup failed");
> +
> + while ((commit = get_revision(&rev)) && !found_unpushed_submodule) {
Why are we checking all commits that are not in "any of the remotes" (I am
referring to "--not --remotes" in the argv[]) here, instead of all commits
that are not in "the remote we are pushing this superproject commit to"?
At this point in the codepath don't you (or you may not---I didn't check)
already know where you are pushing?
> +test_expect_success 'push succeeds if submodule has no remote and is on the first superproject commit' '
> + mkdir a &&
> + (
> + cd a &&
> + git init --bare
> + ) &&
Perhaps
git init --bare a
would do?
^ permalink raw reply
* Re: [PATCH] git-svn: enable platform-specific authentication
From: Matthijs Kooijman @ 2011-08-09 21:06 UTC (permalink / raw)
To: Eric Wong; +Cc: Gustav Munkby, git, Edward Rudd, Carsten Bormann
In-Reply-To: <20110518195710.GA10697@dcvr.yhbt.net>
[-- Attachment #1: Type: text/plain, Size: 3890 bytes --]
Hey folks,
> > Use the platform-specific authentication providers that are
> > exposed to subversion bindings starting with subversion 1.6.
>
> This came up several months ago, I understand there were some issues
> with the SVN Perl bindings. Cc-ing interested parties.
I missed the CC, sorry for that.
> > sub _auth_providers () {
> > [
> > + $SVN::Core::VERSION lt '1.6' ? () :
> > + @{SVN::Core::auth_get_platform_specific_client_providers(
> > + undef,undef)},
>
> I think it needs to take into account the config from
> SVN::Core::config_get_config, otherwise people with non-standard SVN
> configurations could get locked out. I seem to recall this was the
> broken part in the SVN Perl bindings, but one of the Cc-ed parties would
> know for sure.
Indeed, but a proposed patch by Eric for this did not work. I solved the
problem quite some time ago, but apparently I never sent out the
solution (I think I got distracted by trying to get a passphrase prompt
to unlock locked keychains). I couldn't find my fixes anymore either,
but I think I've managed to reproduce them just now.
Some basic testing shows below patch works, but I think it might need
some more testing and work. At least the below patch allows for example
to disable the gnome-keyring provider from a different svn config
directory by passing --config-dir /some/path to git-svn (which is not
possible using above patch passing undef, which will only read from
~/.subversion).
Using strace, I did notice that git-svn still reads stuff
from ~/.subversion/auth/svn.ssl.server/ and
.subversion/auth/svn.simple/, but I couldn't exactly find why this is
right away. In any case, it also happens without this patch applied, so
I guess it's a completely separate issue.
As for the actual patch, notice that config_get_config returns a hash
that consists again of a "config" and "servers" patch. Previous attempts
at this patch passed the entire hash to
auth_get_platform_specific_client_providers, but it only wants the
"client" part. It's a bit confusing until you realize that the
config_get_config return value represents your ~/.subversion directory,
which again contains a "config" and "servers" file.
I'm not 100% sure this patch is correct as it is now. I hope to get
another look at my "automatically unlock keychain" work tomorrow,
in case there are some hints about flaws in this patch there. In the
meanwhile, feedback on this patch is welcome.
Gr.
Matthijs
diff --git a/git-svn.perl b/git-svn.perl
index da3fea8..6dc5196 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4916,7 +4916,7 @@ BEGIN {
}
sub _auth_providers () {
- [
+ my @rv = (
SVN::Client::get_simple_provider(),
SVN::Client::get_ssl_server_trust_file_provider(),
SVN::Client::get_simple_prompt_provider(
@@ -4932,7 +4932,23 @@ sub _auth_providers () {
\&Git::SVN::Prompt::ssl_server_trust),
SVN::Client::get_username_prompt_provider(
\&Git::SVN::Prompt::username, 2)
- ]
+ );
+
+ # earlier 1.6.x versions would segfault, and <= 1.5.x didn't have
+ # this function
+ if ($SVN::Core::VERSION gt '1.6.12') {
+ my $config = SVN::Core::config_get_config($config_dir);
+ my ($p, @a);
+ # config_get_config returns all config files from
+ # ~/.subversion, auth_get_platform_specific_client_providers
+ # just wants the config "file".
+ @a = ($config->{'config'}, undef);
+ $p = SVN::Core::auth_get_platform_specific_client_providers(@a);
+ # Insert the return value from
+ # auth_get_platform_specific_providers
+ unshift @rv, @$p;
+ }
+ \@rv;
}
sub escape_uri_only {
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply related
* Re: [PATCH v3 1/1] bisect: add support for bisecting bare repositories
From: Jon Seymour @ 2011-08-09 23:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, chriscool, j6t, jnareb, jrnieder
In-Reply-To: <7vty9qsdhp.fsf@alter.siamese.dyndns.org>
On Wed, Aug 10, 2011 at 3:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jon Seymour <jon.seymour@gmail.com> writes:
>
>> This extension to js/bisect-no-checkout adds support for bisecting bare repositories.
>>
>> It does this by relaxing the requirement that git bisect is invoked in a repository with a working tree and by defaulting to --no-checkout in the case of a bare repository.
>>
>> Two tests are included to demonstrate this behaviour.
>
> I'd wrap the long line myself but next time please be careful.
>
>> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
>> index 4fb7d11..62125ec 100755
>> --- a/t/t6030-bisect-porcelain.sh
>> +++ b/t/t6030-bisect-porcelain.sh
>> @@ -592,6 +592,37 @@ test_expect_success 'erroring out when using bad path parameters' '
>> grep "bad path parameters" error.txt
>> '
>>
>> +test_expect_success 'test bisection on bare repo - --no-checkout specified' '
>> + git clone --bare . bare.nocheckout &&
>> + (
>> + cd bare.nocheckout &&
>> + git bisect start --no-checkout &&
>> + git bisect good $HASH1 &&
>> + git bisect bad $HASH4 &&
>> + git bisect run eval \
>> + "test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \
>> + >../nocheckout.log &&
>> + git bisect reset
>> + ) &&
>> + grep "$HASH3 is the first bad commit" nocheckout.log
>> +'
>
> git-bisect does not have any cd_to_toplevel so with a working tree you can
> run a bisection from a subdirectory, it seems. I wonder what would happen
> if you did "cd bare.nocheckout/objects" or something silly like that.
>
> Just wondering...
>
Well, nothing obviously breaks...
expecting success:
git clone --bare . bare.silly &&
(
cd bare.silly/objects &&
git bisect start &&
git bisect good $HASH1 &&
git bisect bad $HASH4 &&
git bisect run eval \
"test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \
>../../silly.log &&
git bisect reset
) &&
grep "$HASH3 is the first bad commit" silly.log
Cloning into bare repository bare.silly...
done.
Bisecting: 0 revisions left to test after this (roughly 1 step)
[3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for
git> into <hello>.
3de952f2416b6084f557ec417709eac740c6818c is the first bad commit
^ permalink raw reply
* [GSoC update] Sequencer: Thoughts on future work
From: Ramkumar Ramachandra @ 2011-08-10 1:24 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
Daniel Barkalow, Jeff King
Hi,
It looks like rr/revert-cherry-pick-continue (also referred to as
sequencer-stable in other places) is doing well, but there's just
another week before the end of the SoC. From my experience of working
on the new series, sequencer-stable lays a good foundation to make
future work really easy; it especially makes it easy to split up work
into tiny bits, so that a huge tangled-up series won't be required.
However, even in the best case, I can only get a few reviews and
iterate over my new series a few times before the end of the SoC.
I'll post a mini-series asking for comments soon.
Also, I suspect that academic pressure will be significantly lower
since this is my final year in college. I hope to write a few more
features into the sequencer, and pursue lots of new problems this
year.
Thanks for reading.
-- Ram
^ permalink raw reply
* About git-diff
From: Luiz Ramos @ 2011-08-10 1:52 UTC (permalink / raw)
To: git
Hello,
just a doubt, but it can also be a bug.
Suppose there is a project stored in git, and it has two branches. In the branch b1, there is a single file, file_1. In the branch b2, there is a single file file_2.
Suppose also I am using branch b1 above (that is, "git checkout b1" was previously run), but the file file_2 was left in the working directory along with file_1.
If I run:
$ git diff --cached b2 ./
The results will point that the file file_1 is added, as it is in the index and not in the tree b2. It will also point that file file_2 is removed, as it is listed in the tree b2, but not in the index. No doubts here.
If I run:
$ git diff b2 ./
that is, the "non-cached" version, it will show the same results. This is confusing IMHO, because the git-diff manual suggests that invocation should render the difference between the named tree contents and the working directory. In the working directory, only to recall, file_1 and file_2 are both present and with good versions. In my understanding, the command should report that file_1 is in excess in the working directory, relative to b2, and report nothing about file_2, as it is in the same version as the sample in the tree b2.
This doesn't seem to be the same thing git-diff-index manual states, however. The manual gets more deep into the details, and it's not so easy to understand it unless one knows a lot of the inner commands, which does not apply to me. In my basic reading, it seems that behind the scenes, git-diff-index is what is run in this case, and the fact that file_2 is not in the tree associated to b1 is a relevant thing in this case. So, the index seem to matter, and if I try to do it, a previous "git update-index" should be done.
I may be wrong, but as a user, I'd like to have the non-cached command version comparing tree and working directory, as the manual of git-diff suggests, eventually helped by the index, but not limited to it. Because if I'd like to compare with index, I would try it using --cached instead.
Please accept my apologies if there is some newbie mistake here.
Thanks,
Luiz Ramos
São Paulo - Brazil
^ permalink raw reply
* Re: [PATCH 2/2] On Cygwin support both UNIX and DOS style path-names
From: Vijay Lakshminarayanan @ 2011-08-10 2:44 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Pascal Obry, git
In-Reply-To: <4E416D4A.40602@ramsay1.demon.co.uk>
Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> Pascal Obry wrote:
>> In fact Cygwin supports both, so make Git agree with this.
>> The failing case is when a file is committed in a sub-dir of the
>> repository using a log message from a file specified with a DOS
>> style path-name. To reproduce:
>>
>> $ cd src
>> $ git commit -F c:\tmp\log.txt file.c
>> fatal: could not read log file 'src/c:\tmp\log.txt': No such file \
>> or directory.
>
> Hmm, are you using bash or cmd.exe? Using bash I get the following:
>
> $ cd src
> $ git commut -F c:\tmp\log.txt file.c
> fatal: could not read file 'src/c:tmplog.txt': No such file or directory
> $
>
> Which is what I would expect of (any) posix shell, viz:
>
> $ ls c:\
> > ^C
> $ ls c:\\
> AUTOEXEC.BAT* NTDETECT.COM* WATCOM/ msysgit/
> CMPNENTS/ Program Files/ WINDOWS/ msysgit-old/
> CONFIG.SYS* RECYCLER/ boot.ini* ntldr*
> Documents and Settings/ SUPPORT/ cygwin/ pagefile.sys
> I386/ SWSTAMP.TXT* cygwintemp/ ssl/
> IO.SYS* System Volume Information/ dm/ uname/
> MSDOS.SYS* TOOLSCD/ dm840/ zlib/
> MSOCache/ VALUEADD/ hiberfil.sys
> $
FWIW, I use git on Cygwin and it works quite well. Cygwin supports
Windows paths when quoted. So
$ ls 'C:\Users\vijay\Desktop'
works as expected. This is very useful when you're copying paths from,
say, Windows Explorer, to Cygwin.
> ATB,
> Ramsay Jones
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Cheers
~vijay
Gnus should be more complicated.
^ permalink raw reply
* Re: Tracked files become untracked in some obscure scenario
From: Jeff King @ 2011-08-10 2:51 UTC (permalink / raw)
To: Stéphane Corlosquet; +Cc: git
In-Reply-To: <CAGR+nnFM+Pr29h4POarx5rQdrTuMvtwwZLwVPZuChSDKQjLg4g@mail.gmail.com>
On Tue, Aug 09, 2011 at 03:59:08PM -0400, Stéphane Corlosquet wrote:
> I'm encountering a very strange bug where some directory containing
> one file becomes untracked when it should be tracked. I originally
> found this bug in a bigger repository, but I isolated the culprit
> files into a small tarball which you can download from
> http://files.openspring.net/tmp/gitbug-20110809.tgz
>
> This bug occurs on the following three configs:
> Mac OS X Snow Leopard with git 1.7.4.1
> Mac OS X Snow Leopard with git 1.7.5
> Mac OS X Lion with git 1.7.4.4
> I could not reproduce on Ubuntu.
I suspect this may be another case of this bug:
http://thread.gmane.org/gmane.comp.version-control.git/173612/focus=174634
Does the patch posted in that thread[1] make the problem go away?
-Peff
[1] The actual patch is in:
http://article.gmane.org/gmane.comp.version-control.git/174636
^ permalink raw reply
* Re: git-archive's wrong documentation: really write pax rather than tar
From: Hin-Tak Leung @ 2011-08-10 3:08 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, git
In-Reply-To: <4E3D8EA9.3000609@lsrfire.ath.cx>
--- On Sat, 6/8/11, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> That doesn't sound good. Looking at the R source,
> however, I can see
> that they use a two different algorithms to compute the
> checksum than
> the one specified by POSIX (even though I don't fully
> understand what it
> actually is their doing, since I don't know R). So
> worry too much about
> the warning; as long e.g. "tar tf <file>" doesn't
> complain your archive
> should be intact.
I filed the bug,
https://bugs.r-project.org/bugzilla3/show_bug.cgi?id=14654
and they have fixed it bug has a few comments to make:
---------------
Fixed in R-devel and patched (your checksum field has more than 6 digits which
is highly unusual [since it can't be larger than 6 digits] but technically
allowable).
I should add that the original tar format mandated that checksums are
terminated by NUL SPACE, so in that sense your tar file is invalid (there can't
be more than 6 digits since the checksum field consists of 8 bytes). untar2
will now be more forgiving, but whatever program created that tar file should
be fixed.
-----------------
Please feel free to respond directly at the R bug tracking system, or I can cut-and-paste bits of e-mails also...
^ permalink raw reply
* [RFC PATCH 0/5] Some preliminary work based on sequencer-stable
From: Ramkumar Ramachandra @ 2011-08-10 9:55 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
Daniel Barkalow, Jeff King
Hi,
The series is quite unpolished at the moment, but I'm sending it to
the list because I want early feedback before I make mypoic decisions
and waste work. Tests and rough commit messages included :)
The first patch is something I should've done earlier -- it makes
debugging much easier. Should it go into maint? The last patch is the
most important, and everything else is just building towards it. With
this series, we should now be able to abort at the last-but-one
commit, since 'git commit' actually cleans up the sequencer state now.
Note: All the SHA-1 references in the commit messages will change
after sequencer-stable is merged. I hope it's clear for the moment.
Thanks for reading.
-- Ram
Ramkumar Ramachandra (5):
revert: Don't remove the sequencer state on error
sequencer.h: Move data structures
revert: Allow mixed pick and revert instructions
sequencer: Expose code that handles files in .git/sequencer
sequencer: Remove sequencer state after final commit
builtin/commit.c | 5 +
builtin/revert.c | 417 +++------------------------------------
sequencer.c | 323 ++++++++++++++++++++++++++++++
sequencer.h | 53 +++++
t/t3510-cherry-pick-sequence.sh | 62 ++++++-
5 files changed, 471 insertions(+), 389 deletions(-)
--
1.7.6.351.gb35ac.dirty
^ permalink raw reply
* [PATCH 1/5] revert: Don't remove the sequencer state on error
From: Ramkumar Ramachandra @ 2011-08-10 9:55 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
Daniel Barkalow, Jeff King
In-Reply-To: <1312970151-18906-1-git-send-email-artagnon@gmail.com>
The cherry-pick/ revert machinery now removes the sequencer state when
do_pick_commit returns a non-zero, and when only one instruction is
left in the todo_list. Since do_pick_commit has a way to distinguish
errors from conflicts using the signed-ness of the return value,
utilize this to ensure that the sequencer state is only removed when
there's a conflict and there is only one instruction left in the
todo_list.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 8b452e8..a548a14 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -929,10 +929,10 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
save_todo(cur, opts);
res = do_pick_commit(cur->item, opts);
if (res) {
- if (!cur->next)
+ if (!cur->next && res > 0)
/*
- * An error was encountered while
- * picking the last commit; the
+ * A conflict was encountered while
+ * picking the last commit. The
* sequencer state is useless now --
* the user simply needs to resolve
* the conflict and commit
--
1.7.6.351.gb35ac.dirty
^ permalink raw reply related
* [PATCH 2/5] sequencer.h: Move data structures
From: Ramkumar Ramachandra @ 2011-08-10 9:55 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
Daniel Barkalow, Jeff King
In-Reply-To: <1312970151-18906-1-git-send-email-artagnon@gmail.com>
Prepare to move more stuff into generalized sequencer.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 55 ++++++++++++++---------------------------------------
sequencer.h | 25 ++++++++++++++++++++++++
2 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index a548a14..81502d4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,43 +39,18 @@ static const char * const cherry_pick_usage[] = {
NULL
};
-enum replay_action { REVERT, CHERRY_PICK };
-enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
-
-struct replay_opts {
- enum replay_action action;
- enum replay_subcommand subcommand;
-
- /* Boolean options */
- int edit;
- int record_origin;
- int no_commit;
- int signoff;
- int allow_ff;
- 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;
-};
-
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
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,
@@ -154,7 +129,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"),
@@ -353,7 +328,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."));
@@ -532,7 +507,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
defmsg = git_pathdup("MERGE_MSG");
- if (opts->action == REVERT) {
+ if (opts->action == REPLAY_REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -575,7 +550,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
write_cherry_pick_head(commit);
}
- if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
+ if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
head, &msgbuf, opts);
write_message(&msgbuf, defmsg);
@@ -594,7 +569,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
if (res) {
- error(opts->action == REVERT
+ error(opts->action == REPLAY_REVERT
? _("could not revert %s... %s")
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
@@ -618,7 +593,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);
@@ -680,7 +655,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
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 *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
for (cur = todo_list; cur; cur = cur->next) {
sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
@@ -700,11 +675,11 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
char *p, *q;
if (!prefixcmp(start, "pick ")) {
- action = CHERRY_PICK;
+ action = REPLAY_PICK;
insn_len = strlen("pick");
p = start + insn_len + 1;
} else if (!prefixcmp(start, "revert ")) {
- action = REVERT;
+ action = REPLAY_REVERT;
insn_len = strlen("revert");
p = start + insn_len + 1;
} else
@@ -723,7 +698,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
*/
if (action != opts->action) {
const char *action_str;
- action_str = action == REVERT ? "revert" : "cherry-pick";
+ action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
error(_("Cannot %s during a %s"), action_str, action_name(opts));
return NULL;
}
@@ -989,7 +964,7 @@ static int pick_revisions(struct replay_opts *opts)
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"));
}
@@ -1009,7 +984,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);
@@ -1024,7 +999,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 905d295..0b5d94e 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -7,6 +7,31 @@
#define SEQ_TODO_FILE "sequencer/todo"
#define SEQ_OPTS_FILE "sequencer/opts"
+enum replay_action { REPLAY_REVERT, REPLAY_PICK };
+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
+
+struct replay_opts {
+ enum replay_action action;
+ enum replay_subcommand subcommand;
+
+ /* Boolean options */
+ int edit;
+ int record_origin;
+ int no_commit;
+ int signoff;
+ int allow_ff;
+ 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;
+};
+
/*
* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
* any errors. Intended to be used by 'git reset'.
--
1.7.6.351.gb35ac.dirty
^ permalink raw reply related
* [PATCH 3/5] revert: Allow mixed pick and revert instructions
From: Ramkumar Ramachandra @ 2011-08-10 9:55 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
Daniel Barkalow, Jeff King
In-Reply-To: <1312970151-18906-1-git-send-email-artagnon@gmail.com>
Change the way the instruction parser works, allowing arbitrary
(action, operand) pairs to be parsed. So 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.
Helped-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 110 +++++++++++++++++++--------------------
sequencer.h | 6 ++
t/t3510-cherry-pick-sequence.sh | 58 ++++++++++++++++++++
3 files changed, 117 insertions(+), 57 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 81502d4..d48a92e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -639,89 +639,84 @@ static void read_and_refresh_cache(struct replay_opts *opts)
* assert(commit_list_count(list) == 2);
* return list;
*/
-struct commit_list **commit_list_append(struct commit *commit,
- struct commit_list **next)
+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(struct replay_insn_list));
+ 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;
+ struct replay_insn_list *cur = NULL;
struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
const char *sha1_abbrev = NULL;
- const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
+ const char *action_str;
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))
+ action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+
+ sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+ if (get_message(cur->operand, &msg))
return error(_("Cannot get commit message for %s"), sha1_abbrev);
strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
}
return 0;
}
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static int parse_insn_line(char *start, enum replay_action *action,
+ struct commit **operand)
{
unsigned char commit_sha1[20];
char sha1_abbrev[40];
- enum replay_action action;
- int insn_len = 0;
+ int keyword_len;
char *p, *q;
if (!prefixcmp(start, "pick ")) {
- action = REPLAY_PICK;
- insn_len = strlen("pick");
- p = start + insn_len + 1;
+ *action = REPLAY_PICK;
+ keyword_len = strlen("pick");
+ p = start + keyword_len + 1;
} else if (!prefixcmp(start, "revert ")) {
- action = REPLAY_REVERT;
- insn_len = strlen("revert");
- p = start + insn_len + 1;
+ *action = REPLAY_REVERT;
+ keyword_len = strlen("revert");
+ p = start + keyword_len + 1;
} else
- return NULL;
+ return -1;
q = strchr(p, ' ');
if (!q)
- return NULL;
+ return -1;
q++;
strlcpy(sha1_abbrev, p, q - p);
- /*
- * 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 == REPLAY_REVERT ? "revert" : "cherry-pick";
- error(_("Cannot %s during a %s"), action_str, action_name(opts));
- return NULL;
- }
-
if (get_sha1(sha1_abbrev, commit_sha1) < 0)
- return NULL;
+ return -1;
+
+ *operand = lookup_commit_reference(commit_sha1);
+ if (!*operand)
+ return -1;
- return lookup_commit_reference(commit_sha1);
+ 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;
+ enum replay_action action;
+ struct commit *operand;
char *p = buf;
int i;
for (i = 1; *p; i++) {
- commit = parse_insn_line(p, opts);
- if (!commit)
+ if (parse_insn_line(p, &action, &operand) < 0)
return error(_("Could not parse line %d."), i);
- next = commit_list_append(commit, next);
+ next = replay_insn_list_append(action, operand, next);
p = strchrnul(p, '\n');
if (*p)
p++;
@@ -731,8 +726,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;
@@ -748,7 +742,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);
@@ -797,18 +791,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 commit *operand;
+ struct replay_insn_list **next;
prepare_revs(&revs, opts);
next = todo_list;
- while ((commit = get_revision(&revs)))
- next = commit_list_append(commit, next);
+ while ((operand = get_revision(&revs)))
+ next = replay_insn_list_append(opts->action, operand, next);
}
static int create_seq_dir(void)
@@ -837,7 +831,7 @@ static void save_head(const char *head)
die(_("Error wrapping up %s."), head_file);
}
-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;
@@ -845,7 +839,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);
@@ -889,9 +883,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);
@@ -901,8 +896,9 @@ 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);
+ opts->action = cur->action;
+ res = do_pick_commit(cur->operand, opts);
if (res) {
if (!cur->next && res > 0)
/*
@@ -927,7 +923,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);
@@ -944,7 +940,7 @@ static int pick_revisions(struct replay_opts *opts)
if (!file_exists(git_path(SEQ_TODO_FILE)))
goto error;
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))
diff --git a/sequencer.h b/sequencer.h
index 0b5d94e..01caa8a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -32,6 +32,12 @@ struct replay_opts {
size_t xopts_nr, xopts_alloc;
};
+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 3bca2b3..f85372c 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -211,4 +211,62 @@ test_expect_success 'malformed instruction sheet 2' '
test_must_fail git cherry-pick --continue
'
+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.6.351.gb35ac.dirty
^ permalink raw reply related
* [PATCH 5/5] sequencer: Remove sequencer state after final commit
From: Ramkumar Ramachandra @ 2011-08-10 9:55 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
Daniel Barkalow, Jeff King
In-Reply-To: <1312970151-18906-1-git-send-email-artagnon@gmail.com>
Since d3f4628e (revert: Remove sequencer state when no commits are
pending, 2011-07-06), the sequencer removes the sequencer state before
the final commit is actually completed. This design is inherently
flawed, as it will not allow the user to abort the sequencer operation
at that stage. Instead, write and expose a new function to count the
number of commits left in the instruction sheet; use this in
builtin/commit.c to remove the sequencer state when a commit has
successfully completed and there is only one instruction left in the
sheet.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/commit.c | 5 +++++
builtin/revert.c | 12 +-----------
sequencer.c | 14 ++++++++++++++
sequencer.h | 1 +
t/t3510-cherry-pick-sequence.sh | 4 ++--
5 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index e1af9b1..7ee4269 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -26,6 +26,7 @@
#include "unpack-trees.h"
#include "quote.h"
#include "submodule.h"
+#include "sequencer.h"
static const char * const builtin_commit_usage[] = {
"git commit [options] [--] <filepattern>...",
@@ -1521,6 +1522,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
unlink(git_path("MERGE_MODE"));
unlink(git_path("SQUASH_MSG"));
+ /* Remove sequencer state if we just finished the last insn */
+ if (sequencer_count_todo() == 1)
+ remove_sequencer_state(1);
+
if (commit_index_files())
die (_("Repository has been updated, but unable to write\n"
"new_index file. Check that disk is not full or quota is\n"
diff --git a/builtin/revert.c b/builtin/revert.c
index 491c10a..718ba86 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -581,18 +581,8 @@ static int pick_commits(struct replay_insn_list *todo_list,
sequencer_save_todo(cur);
opts->action = cur->action;
res = do_pick_commit(cur->operand, opts);
- if (res) {
- if (!cur->next && res > 0)
- /*
- * A conflict was encountered while
- * picking the last commit. The
- * sequencer state is useless now --
- * the user simply needs to resolve
- * the conflict and commit
- */
- remove_sequencer_state(0);
+ if (res)
return res;
- }
}
/*
diff --git a/sequencer.c b/sequencer.c
index 2993846..0ad1da6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -326,3 +326,17 @@ void sequencer_save_opts(struct replay_opts *opts)
opts->xopts[i], "^$", 0);
}
}
+
+int sequencer_count_todo(void)
+{
+ struct replay_insn_list *todo_list = NULL;
+ struct replay_insn_list *cur;
+ int insn_count = 0;
+
+ if (!file_exists(git_path(SEQ_TODO_FILE)))
+ return 0;
+ sequencer_read_todo(&todo_list);
+ for (cur = todo_list; cur; cur = cur->next)
+ insn_count += 1;
+ return insn_count;
+}
diff --git a/sequencer.h b/sequencer.h
index a5b951d..ca133e2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -68,5 +68,6 @@ int sequencer_create_dir(void);
void sequencer_save_head(const char *head);
void sequencer_save_todo(struct replay_insn_list *todo_list);
void sequencer_save_opts(struct replay_opts *opts);
+int sequencer_count_todo(void);
#endif
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index f85372c..832baa0 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -82,13 +82,13 @@ test_expect_success '--reset cleans up sequencer state' '
test_path_is_missing .git/sequencer
'
-test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
+test_expect_success 'final commit cleans up sequencer state' '
pristine_detach initial &&
test_must_fail git cherry-pick base..picked &&
- test_path_is_missing .git/sequencer &&
echo "resolved" >foo &&
git add foo &&
git commit &&
+ test_path_is_missing .git/sequencer &&
{
git rev-list HEAD |
git diff-tree --root --stdin |
--
1.7.6.351.gb35ac.dirty
^ permalink raw reply related
* [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer
From: Ramkumar Ramachandra @ 2011-08-10 9:55 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
Daniel Barkalow, Jeff King
In-Reply-To: <1312970151-18906-1-git-send-email-artagnon@gmail.com>
Move the saving and parsing functions for files in '.git/sequencer'
from 'builtin/revert.c' into 'sequencer.c' and expose them using a
sane API. This is the first step towards a generalized sequencer.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 332 +----------------------------------------------------
sequencer.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++++++
sequencer.h | 21 ++++
3 files changed, 337 insertions(+), 325 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index d48a92e..491c10a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -5,7 +5,6 @@
#include "tag.h"
#include "run-command.h"
#include "exec_cmd.h"
-#include "utf8.h"
#include "parse-options.h"
#include "cache-tree.h"
#include "diff.h"
@@ -46,8 +45,6 @@ static const char *action_name(const struct replay_opts *opts)
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 == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
@@ -189,80 +186,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
opts->commit_argv = argv;
}
-struct commit_message {
- char *parent_label;
- const char *label;
- const char *subject;
- char *reencoded_message;
- const char *message;
-};
-
-static int get_message(struct commit *commit, struct commit_message *out)
-{
- const char *encoding;
- const char *abbrev, *subject;
- int abbrev_len, subject_len;
- char *q;
-
- if (!commit->buffer)
- return -1;
- encoding = get_encoding(commit->buffer);
- if (!encoding)
- encoding = "UTF-8";
- if (!git_commit_encoding)
- git_commit_encoding = "UTF-8";
-
- out->reencoded_message = NULL;
- out->message = commit->buffer;
- if (strcmp(encoding, git_commit_encoding))
- out->reencoded_message = reencode_string(commit->buffer,
- git_commit_encoding, encoding);
- if (out->reencoded_message)
- out->message = out->reencoded_message;
-
- abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
- abbrev_len = strlen(abbrev);
-
- subject_len = find_commit_subject(out->message, &subject);
-
- out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
- strlen("... ") + subject_len + 1);
- q = out->parent_label;
- q = mempcpy(q, "parent of ", strlen("parent of "));
- out->label = q;
- q = mempcpy(q, abbrev, abbrev_len);
- q = mempcpy(q, "... ", strlen("... "));
- out->subject = q;
- q = mempcpy(q, subject, subject_len);
- *q = '\0';
- return 0;
-}
-
static void free_message(struct commit_message *msg)
{
free(msg->parent_label);
free(msg->reencoded_message);
}
-static char *get_encoding(const char *message)
-{
- const char *p = message, *eol;
-
- while (*p && *p != '\n') {
- for (eol = p + 1; *eol && *eol != '\n'; eol++)
- ; /* do nothing */
- if (!prefixcmp(p, "encoding ")) {
- char *result = xmalloc(eol - 8 - p);
- strlcpy(result, p + 9, eol - 8 - p);
- return result;
- }
- p = eol;
- if (*p == '\n')
- p++;
- }
- return NULL;
-}
-
static void write_cherry_pick_head(struct commit *commit)
{
int fd;
@@ -494,7 +423,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
return error(_("%s: cannot parse parent commit %s"),
action_name(opts), sha1_to_hex(parent->object.sha1));
- if (get_message(commit, &msg) != 0)
+ if (get_commit_message(commit, &msg) != 0)
return error(_("Cannot get commit message for %s"),
sha1_to_hex(commit->object.sha1));
@@ -622,175 +551,6 @@ static void read_and_refresh_cache(struct replay_opts *opts)
rollback_lock_file(&index_lock);
}
-/*
- * Append a commit to the end of the commit_list.
- *
- * next starts by pointing to the variable that holds the head of an
- * empty commit_list, and is updated to point to the "next" field of
- * the last item on the list as new commits are appended.
- *
- * Usage example:
- *
- * struct commit_list *list;
- * struct commit_list **next = &list;
- *
- * next = commit_list_append(c1, next);
- * next = commit_list_append(c2, next);
- * assert(commit_list_count(list) == 2);
- * return list;
- */
-struct replay_insn_list **replay_insn_list_append(enum replay_action action,
- struct commit *operand,
- struct replay_insn_list **next)
-{
- struct replay_insn_list *new = xmalloc(sizeof(struct replay_insn_list));
- new->action = action;
- new->operand = operand;
- *next = new;
- new->next = NULL;
- return &new->next;
-}
-
-static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
-{
- struct replay_insn_list *cur = NULL;
- struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
- const char *sha1_abbrev = NULL;
- const char *action_str;
-
- for (cur = todo_list; cur; cur = cur->next) {
- action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
-
- sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
- if (get_message(cur->operand, &msg))
- return error(_("Cannot get commit message for %s"), sha1_abbrev);
- strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
- }
- return 0;
-}
-
-static int parse_insn_line(char *start, enum replay_action *action,
- struct commit **operand)
-{
- unsigned char commit_sha1[20];
- char sha1_abbrev[40];
- int keyword_len;
- char *p, *q;
-
- if (!prefixcmp(start, "pick ")) {
- *action = REPLAY_PICK;
- keyword_len = strlen("pick");
- p = start + keyword_len + 1;
- } else if (!prefixcmp(start, "revert ")) {
- *action = REPLAY_REVERT;
- keyword_len = strlen("revert");
- p = start + keyword_len + 1;
- } else
- return -1;
-
- q = strchr(p, ' ');
- if (!q)
- return -1;
- q++;
-
- strlcpy(sha1_abbrev, p, q - p);
-
- if (get_sha1(sha1_abbrev, commit_sha1) < 0)
- return -1;
-
- *operand = lookup_commit_reference(commit_sha1);
- if (!*operand)
- return -1;
-
- return 0;
-}
-
-static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
-{
- struct replay_insn_list **next = todo_list;
- enum replay_action action;
- struct commit *operand;
- char *p = buf;
- int i;
-
- for (i = 1; *p; i++) {
- if (parse_insn_line(p, &action, &operand) < 0)
- return error(_("Could not parse line %d."), i);
- next = replay_insn_list_append(action, operand, next);
- p = strchrnul(p, '\n');
- if (*p)
- p++;
- }
- if (!*todo_list)
- return error(_("No commits parsed."));
- return 0;
-}
-
-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;
- int fd, res;
-
- fd = open(todo_file, O_RDONLY);
- if (fd < 0)
- die_errno(_("Could not open %s."), todo_file);
- if (strbuf_read(&buf, fd, 0) < 0) {
- close(fd);
- strbuf_release(&buf);
- die(_("Could not read %s."), todo_file);
- }
- close(fd);
-
- res = parse_insn_buffer(buf.buf, todo_list);
- strbuf_release(&buf);
- if (res)
- die(_("Unusable instruction sheet: %s"), todo_file);
-}
-
-static int populate_opts_cb(const char *key, const char *value, void *data)
-{
- struct replay_opts *opts = data;
- int error_flag = 1;
-
- if (!value)
- error_flag = 0;
- else if (!strcmp(key, "options.no-commit"))
- opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.edit"))
- opts->edit = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.signoff"))
- opts->signoff = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.record-origin"))
- opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.allow-ff"))
- opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.mainline"))
- opts->mainline = git_config_int(key, value);
- else if (!strcmp(key, "options.strategy"))
- git_config_string(&opts->strategy, key, value);
- else if (!strcmp(key, "options.strategy-option")) {
- ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
- opts->xopts[opts->xopts_nr++] = xstrdup(value);
- } else
- return error(_("Invalid key: %s"), key);
-
- if (!error_flag)
- return error(_("Invalid value for %s: %s"), key, value);
-
- return 0;
-}
-
-static void read_populate_opts(struct replay_opts **opts_ptr)
-{
- const char *opts_file = git_path(SEQ_OPTS_FILE);
-
- if (!file_exists(opts_file))
- return;
- if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
- die(_("Malformed options sheet: %s"), opts_file);
-}
-
static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
struct replay_opts *opts)
{
@@ -805,84 +565,6 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
next = replay_insn_list_append(opts->action, operand, next);
}
-static int create_seq_dir(void)
-{
- const char *seq_dir = git_path(SEQ_DIR);
-
- 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);
- return 0;
-}
-
-static void save_head(const char *head)
-{
- const char *head_file = git_path(SEQ_HEAD_FILE);
- static struct lock_file head_lock;
- struct strbuf buf = STRBUF_INIT;
- int fd;
-
- 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);
- if (commit_lock_file(&head_lock) < 0)
- die(_("Error wrapping up %s."), head_file);
-}
-
-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;
- struct strbuf buf = STRBUF_INIT;
- int fd;
-
- fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
- 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);
- die_errno(_("Could not write to %s."), todo_file);
- }
- if (commit_lock_file(&todo_lock) < 0) {
- strbuf_release(&buf);
- die(_("Error wrapping up %s."), todo_file);
- }
- strbuf_release(&buf);
-}
-
-static void save_opts(struct replay_opts *opts)
-{
- const char *opts_file = git_path(SEQ_OPTS_FILE);
-
- if (opts->no_commit)
- git_config_set_in_file(opts_file, "options.no-commit", "true");
- if (opts->edit)
- git_config_set_in_file(opts_file, "options.edit", "true");
- if (opts->signoff)
- git_config_set_in_file(opts_file, "options.signoff", "true");
- if (opts->record_origin)
- git_config_set_in_file(opts_file, "options.record-origin", "true");
- if (opts->allow_ff)
- git_config_set_in_file(opts_file, "options.allow-ff", "true");
- if (opts->mainline) {
- struct strbuf buf = STRBUF_INIT;
- strbuf_addf(&buf, "%d", opts->mainline);
- git_config_set_in_file(opts_file, "options.mainline", buf.buf);
- strbuf_release(&buf);
- }
- if (opts->strategy)
- git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
- if (opts->xopts) {
- int i;
- for (i = 0; i < opts->xopts_nr; i++)
- git_config_set_multivar_in_file(opts_file,
- "options.strategy-option",
- opts->xopts[i], "^$", 0);
- }
-}
-
static int pick_commits(struct replay_insn_list *todo_list,
struct replay_opts *opts)
{
@@ -896,7 +578,7 @@ static int pick_commits(struct replay_insn_list *todo_list,
read_and_refresh_cache(opts);
for (cur = todo_list; cur; cur = cur->next) {
- save_todo(cur);
+ sequencer_save_todo(cur);
opts->action = cur->action;
res = do_pick_commit(cur->operand, opts);
if (res) {
@@ -939,8 +621,8 @@ static int pick_revisions(struct replay_opts *opts)
} else if (opts->subcommand == REPLAY_CONTINUE) {
if (!file_exists(git_path(SEQ_TODO_FILE)))
goto error;
- read_populate_opts(&opts);
- read_populate_todo(&todo_list);
+ sequencer_read_opts(&opts);
+ sequencer_read_todo(&todo_list);
/* Verify that the conflict has been resolved */
if (!index_differs_from("HEAD", 0))
@@ -953,7 +635,7 @@ static int pick_revisions(struct replay_opts *opts)
*/
walk_revs_populate_todo(&todo_list, opts);
- if (create_seq_dir() < 0) {
+ if (sequencer_create_dir() < 0) {
error(_("A cherry-pick or revert is in progress."));
advise(_("Use --continue to continue the operation"));
advise(_("or --reset to forget about it"));
@@ -964,8 +646,8 @@ static int pick_revisions(struct replay_opts *opts)
return error(_("Can't revert as initial commit"));
return error(_("Can't cherry-pick into empty head"));
}
- save_head(sha1_to_hex(sha1));
- save_opts(opts);
+ sequencer_save_head(sha1_to_hex(sha1));
+ sequencer_save_opts(opts);
}
return pick_commits(todo_list, opts);
error:
diff --git a/sequencer.c b/sequencer.c
index bc2c046..2993846 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2,6 +2,8 @@
#include "sequencer.h"
#include "strbuf.h"
#include "dir.h"
+#include "commit.h"
+#include "utf8.h"
void remove_sequencer_state(int aggressive)
{
@@ -17,3 +19,310 @@ void remove_sequencer_state(int aggressive)
strbuf_release(&seq_dir);
strbuf_release(&seq_old_dir);
}
+
+/*
+ * Append a commit to the end of the commit_list.
+ *
+ * next starts by pointing to the variable that holds the head of an
+ * empty commit_list, and is updated to point to the "next" field of
+ * the last item on the list as new commits are appended.
+ *
+ * Usage example:
+ *
+ * struct commit_list *list;
+ * struct commit_list **next = &list;
+ *
+ * next = commit_list_append(c1, next);
+ * next = commit_list_append(c2, next);
+ * assert(commit_list_count(list) == 2);
+ * return list;
+ */
+struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+ struct commit *operand,
+ struct replay_insn_list **next)
+{
+ struct replay_insn_list *new = xmalloc(sizeof(struct replay_insn_list));
+ new->action = action;
+ new->operand = operand;
+ *next = new;
+ new->next = NULL;
+ return &new->next;
+}
+
+static int parse_insn_line(char *start, enum replay_action *action,
+ struct commit **operand)
+{
+ unsigned char commit_sha1[20];
+ char sha1_abbrev[40];
+ int keyword_len;
+ char *p, *q;
+
+ if (!prefixcmp(start, "pick ")) {
+ *action = REPLAY_PICK;
+ keyword_len = strlen("pick");
+ p = start + keyword_len + 1;
+ } else if (!prefixcmp(start, "revert ")) {
+ *action = REPLAY_REVERT;
+ keyword_len = strlen("revert");
+ p = start + keyword_len + 1;
+ } else
+ return -1;
+
+ q = strchr(p, ' ');
+ if (!q)
+ return -1;
+ q++;
+
+ strlcpy(sha1_abbrev, p, q - p);
+
+ if (get_sha1(sha1_abbrev, commit_sha1) < 0)
+ return -1;
+
+ *operand = lookup_commit_reference(commit_sha1);
+ if (!*operand)
+ return -1;
+
+ return 0;
+}
+
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
+{
+ struct replay_insn_list **next = todo_list;
+ enum replay_action action;
+ struct commit *operand;
+ char *p = buf;
+ int i;
+
+ for (i = 1; *p; i++) {
+ if (parse_insn_line(p, &action, &operand) < 0)
+ return error(_("Could not parse line %d."), i);
+ next = replay_insn_list_append(action, operand, next);
+ p = strchrnul(p, '\n');
+ if (*p)
+ p++;
+ }
+ if (!*todo_list)
+ return error(_("No commits parsed."));
+ return 0;
+}
+
+void sequencer_read_todo(struct replay_insn_list **todo_list)
+{
+ const char *todo_file = git_path(SEQ_TODO_FILE);
+ struct strbuf buf = STRBUF_INIT;
+ int fd, res;
+
+ fd = open(todo_file, O_RDONLY);
+ if (fd < 0)
+ die_errno(_("Could not open %s."), todo_file);
+ if (strbuf_read(&buf, fd, 0) < 0) {
+ close(fd);
+ strbuf_release(&buf);
+ die(_("Could not read %s."), todo_file);
+ }
+ close(fd);
+
+ res = parse_insn_buffer(buf.buf, todo_list);
+ strbuf_release(&buf);
+ if (res)
+ die(_("Unusable instruction sheet: %s"), todo_file);
+}
+
+static int populate_opts_cb(const char *key, const char *value, void *data)
+{
+ struct replay_opts *opts = data;
+ int error_flag = 1;
+
+ if (!value)
+ error_flag = 0;
+ else if (!strcmp(key, "options.no-commit"))
+ opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.edit"))
+ opts->edit = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.signoff"))
+ opts->signoff = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.record-origin"))
+ opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.allow-ff"))
+ opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.mainline"))
+ opts->mainline = git_config_int(key, value);
+ else if (!strcmp(key, "options.strategy"))
+ git_config_string(&opts->strategy, key, value);
+ else if (!strcmp(key, "options.strategy-option")) {
+ ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
+ opts->xopts[opts->xopts_nr++] = xstrdup(value);
+ } else
+ return error(_("Invalid key: %s"), key);
+
+ if (!error_flag)
+ return error(_("Invalid value for %s: %s"), key, value);
+
+ return 0;
+}
+
+static char *get_encoding(const char *message)
+{
+ const char *p = message, *eol;
+
+ while (*p && *p != '\n') {
+ for (eol = p + 1; *eol && *eol != '\n'; eol++)
+ ; /* do nothing */
+ if (!prefixcmp(p, "encoding ")) {
+ char *result = xmalloc(eol - 8 - p);
+ strlcpy(result, p + 9, eol - 8 - p);
+ return result;
+ }
+ p = eol;
+ if (*p == '\n')
+ p++;
+ }
+ return NULL;
+}
+
+int get_commit_message(struct commit *commit, struct commit_message *out)
+{
+ const char *encoding;
+ const char *abbrev, *subject;
+ int abbrev_len, subject_len;
+ char *q;
+
+ if (!commit->buffer)
+ return -1;
+ encoding = get_encoding(commit->buffer);
+ if (!encoding)
+ encoding = "UTF-8";
+ if (!git_commit_encoding)
+ git_commit_encoding = "UTF-8";
+
+ out->reencoded_message = NULL;
+ out->message = commit->buffer;
+ if (strcmp(encoding, git_commit_encoding))
+ out->reencoded_message = reencode_string(commit->buffer,
+ git_commit_encoding, encoding);
+ if (out->reencoded_message)
+ out->message = out->reencoded_message;
+
+ abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+ abbrev_len = strlen(abbrev);
+
+ subject_len = find_commit_subject(out->message, &subject);
+
+ out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
+ strlen("... ") + subject_len + 1);
+ q = out->parent_label;
+ q = mempcpy(q, "parent of ", strlen("parent of "));
+ out->label = q;
+ q = mempcpy(q, abbrev, abbrev_len);
+ q = mempcpy(q, "... ", strlen("... "));
+ out->subject = q;
+ q = mempcpy(q, subject, subject_len);
+ *q = '\0';
+ return 0;
+}
+
+void sequencer_read_opts(struct replay_opts **opts_ptr)
+{
+ const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+ if (!file_exists(opts_file))
+ return;
+ if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
+ die(_("Malformed options sheet: %s"), opts_file);
+}
+
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
+{
+ struct replay_insn_list *cur = NULL;
+ struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+ const char *sha1_abbrev = NULL;
+ const char *action_str;
+
+ for (cur = todo_list; cur; cur = cur->next) {
+ action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+
+ sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+ if (get_commit_message(cur->operand, &msg))
+ return error(_("Cannot get commit message for %s"), sha1_abbrev);
+ strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+ }
+ return 0;
+}
+
+int sequencer_create_dir(void)
+{
+ const char *seq_dir = git_path(SEQ_DIR);
+
+ 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);
+ return 0;
+}
+
+void sequencer_save_head(const char *head)
+{
+ const char *head_file = git_path(SEQ_HEAD_FILE);
+ static struct lock_file head_lock;
+ struct strbuf buf = STRBUF_INIT;
+ int fd;
+
+ 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);
+ if (commit_lock_file(&head_lock) < 0)
+ die(_("Error wrapping up %s."), head_file);
+}
+
+void sequencer_save_todo(struct replay_insn_list *todo_list)
+{
+ const char *todo_file = git_path(SEQ_TODO_FILE);
+ static struct lock_file todo_lock;
+ struct strbuf buf = STRBUF_INIT;
+ int fd;
+
+ fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
+ 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);
+ die_errno(_("Could not write to %s."), todo_file);
+ }
+ if (commit_lock_file(&todo_lock) < 0) {
+ strbuf_release(&buf);
+ die(_("Error wrapping up %s."), todo_file);
+ }
+ strbuf_release(&buf);
+}
+
+void sequencer_save_opts(struct replay_opts *opts)
+{
+ const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+ if (opts->no_commit)
+ git_config_set_in_file(opts_file, "options.no-commit", "true");
+ if (opts->edit)
+ git_config_set_in_file(opts_file, "options.edit", "true");
+ if (opts->signoff)
+ git_config_set_in_file(opts_file, "options.signoff", "true");
+ if (opts->record_origin)
+ git_config_set_in_file(opts_file, "options.record-origin", "true");
+ if (opts->allow_ff)
+ git_config_set_in_file(opts_file, "options.allow-ff", "true");
+ if (opts->mainline) {
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addf(&buf, "%d", opts->mainline);
+ git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+ strbuf_release(&buf);
+ }
+ if (opts->strategy)
+ git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+ if (opts->xopts) {
+ int i;
+ for (i = 0; i < opts->xopts_nr; i++)
+ git_config_set_multivar_in_file(opts_file,
+ "options.strategy-option",
+ opts->xopts[i], "^$", 0);
+ }
+}
diff --git a/sequencer.h b/sequencer.h
index 01caa8a..a5b951d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -38,6 +38,17 @@ struct replay_insn_list {
struct replay_insn_list *next;
};
+/* Unrelated commit_message helper */
+struct commit_message {
+ char *parent_label;
+ const char *label;
+ const char *subject;
+ char *reencoded_message;
+ const char *message;
+};
+
+int get_commit_message(struct commit *commit, struct commit_message *out);
+
/*
* Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
* any errors. Intended to be used by 'git reset'.
@@ -48,4 +59,14 @@ struct replay_insn_list {
*/
void remove_sequencer_state(int aggressive);
+struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+ struct commit *operand,
+ struct replay_insn_list **next);
+void sequencer_read_todo(struct replay_insn_list **todo_list);
+void sequencer_read_opts(struct replay_opts **opts_ptr);
+int sequencer_create_dir(void);
+void sequencer_save_head(const char *head);
+void sequencer_save_todo(struct replay_insn_list *todo_list);
+void sequencer_save_opts(struct replay_opts *opts);
+
#endif
--
1.7.6.351.gb35ac.dirty
^ permalink raw reply related
* Re: What's cooking in git.git (Aug 2011, #02; Mon, 8)
From: Jakub Narebski @ 2011-08-10 11:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8vr3v7az.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> --------------------------------------------------
> [Graduated to "master"]
> * jn/gitweb-config-list-case (2011-07-31) 1 commit
> (merged to 'next' on 2011-08-01 at 9268738)
> + gitweb: Git config keys are case insensitive, make config search too
>
> * jn/gitweb-system-config (2011-07-24) 1 commit
> (merged to 'next' on 2011-08-01 at 4941e45)
> + gitweb: Introduce common system-wide settings for convenience
What happened with "[PATCH/RFC 0/6] gitweb: Improve project search"
series from 29.07.2011?
http://thread.gmane.org/gmane.comp.version-control.git/178132
Should I clean it up more to have this RFC series included in 'pu'?
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] Support specific color for a specific remote branches
From: Jeff King @ 2011-08-10 12:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Aviv Eyal, git
In-Reply-To: <7v62m7wpxg.fsf@alter.siamese.dyndns.org>
On Mon, Aug 08, 2011 at 02:31:39PM -0700, Junio C Hamano wrote:
> But in real-life, it is entirely plausible that people with multiple
> integration branches are taking advantage of the simplicity of the old
> layout, i.e.
>
> [remote "origin"]
> fetch = refs/heads/master:refs/heads/origin
> fetch = refs/heads/next:refs/heads/next
> fetch = refs/heads/maint:refs/heads/maint
> fetch = +refs/heads/pu:refs/heads/pu
>
> [...]
>
> Now, for these repositories, is "next" a local branch or a remote one? I
> have a feeling that it might be easier to understand if we label anything
> that you can update with "checkout && commit" a local one for the purpose
> of "branch -r" listing; IOW, the current "git branch -r" classification
> would match this use pattern better, even though refs/heads/next _is_ an
> RHS of a rule to follow others.
Agreed, that really muddies the idea of what is a "remote branch" and
what is not. I think there is a sense among users that "local branches"
and "remote branches" are mutually exclusive sets, even though by some
definitions they may not be (i.e., if you define the former by where in
the refs/ hierarchy they exist, and the latter by being the RHS of a
remote fetch refspec).
> In that sense, I would be entirely happy if the configuration variable
> used in this series were branch.<namepattern>.color and let you specify
>
> [branch "refs/heads"] color = yellow
> [branch "refs/remotes/origin"] color = purple
> [branch "refs/remotes/nitfol"] color = cyan
There are two issues I see with that:
1. Until now, "branch" sections like this have always been about local
branches. What are the rules for defining something like
"branch.refs/remotes/origin/foo.merge"? Knowing how the code works,
it is easy to say it is a noop, as we will never look at it. But I
wonder how it looks from the perspective if a recent git user.
2. Until now, "branch" sections specify full refs, not subsets or
wildcard patterns. What does "branch.refs/heads.merge" mean?
A noop? A wildcard with slightly lesser precedence than
"branch.refs/heads/foo.merge"?
If we are going to go this route, I think it is really about introducing
properties not on branches, but on subsets of the ref namespace. So
might say:
[ref "refs/heads/*"] color = yellow
which says "whenever you are dealing with this part of the refs
namespace, my preferred color is yellow". And "git branch" happens to
respect that (and we could just as easily have a "%(refcolor)" marker in
git-for-each-ref).
I know the difference is subtle, but I just think it removes entirely
the question about what is a branch and what is not. Furthermore, it
naturally extends to other commands (e.g., you could color subsets of
the tag namespace differently), to more complex layouts (e.g., if we
end up moving fetched tags into the refs/remotes namespace eventually),
and to other properties besides color (though I haven't though up any
applications).
> It becomes complicated (and for no good reason, in my opinion; see the
> "next" example above) if you try to tie this with remote.<name> hierarchy,
> as it obviously becomes illogical not to use the "RHS of a fetch refspec"
> logic when we are talking about remote.<name>.
We've been discussing the coloring issue here. But the other thread I
pointed out was about asking "which fetched branches do we have locally
for this remote?". Which is a very reasonable thing to ask, and which
don't do a good job of answering right now. And I think it has to
do the "RHS of a fetch refspec thing".
Right now you can do "git remote show". But:
1. It's very heavyweight. It shows you way more than you want in most
cases, and it touches the network by default (there is a "-n"
option, but touching the network by default makes it pretty
un-git).
2. It's not as easily discoverable as "git branch -r". It's not
unreasonable for users to mentally go through the sequence:
a. "git branch" shows me branches
b. oh, it has an "-r" option for remotes
c. how do I limit to one remote?
I'm not sure what the right solution is. Going from 2b to 2c is a very
natural thing for a user to want to do. But it means jumping from one
definition of "remote" (i.e., everything under "refs/remotes") to
another (the config defined by remote "foo"). In the default config,
that is a natural jump, as they are semantically connected. But they
don't have to be.
-Peff
^ permalink raw reply
* Re: A little help with error?
From: Jeff King @ 2011-08-10 12:21 UTC (permalink / raw)
To: Rusty Dog Ink; +Cc: git
In-Reply-To: <752985D1B23046F994920F9018B53DF8@rustydogink.com>
On Sun, Aug 07, 2011 at 02:16:57PM -0700, Rusty Dog Ink wrote:
> git add --force /Users/chris/Sites/mattSchubert/public/css/browserfix.css => "Crappy Error"
> Yields this error.
> fatal: '/Users/chris/Sites/mattSchubert/public/css/browserfix.css' is outside repository
We can't say if this is reasonable or not without knowing more. Which
directory are you in? Which git repository do you expect to be found?
Which repository does git actually find (try "git rev-parse --git-dir")?
Do you have GIT_WORK_TREE set in the environment, or core.worktree set
in your config?
> This command does gives the same error.
> git add --force /public/css/browserfix.css => "Same error"
> But this command, works. Why?
> git add --force public/css/browserfix.css => "Works yeah"
The top one seems nonsensical, unless your git repository is at one of:
/.git
/public/.git
/public/css/.git
The bottom one makes sense; you are looking at files in the current
directory, which is going to be inside your repository (unless you are
setting some of worktree variables I mentioned above).
Are you trying to do something clever with symlinks to /public? If so,
show us.
-Peff
^ permalink raw reply
* Re: Using git-svn fetch after a directory change with rewrite-root
From: H Krishnan @ 2011-08-10 13:22 UTC (permalink / raw)
To: git
In-Reply-To: <CANWsHyfHtr0EaJtNsDK9UTcmb_AbLg-1jUA-0uWJ-nEeNosb7w@mail.gmail.com>
Hi,
What is the procedure for getting this change into git? Should I contact the
person who submitted the original patch?
Krishnan
^ permalink raw reply
* Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions
From: Jonathan Nieder @ 2011-08-10 15:15 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Git List, Junio C Hamano, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312970151-18906-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Change the way the instruction parser works, allowing arbitrary
> (action, operand) pairs to be parsed. So 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.
Nice. :)
> This patch lays the foundation for extending the parser to support
> more actions.
And why would I want to do that? I think there's a missing "so git
rebase -i can reuse this machinery some day" at the end of the
sentence.
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -639,89 +639,84 @@ static void read_and_refresh_cache(struct replay_opts *opts)
> * assert(commit_list_count(list) == 2);
> * return list;
> */
> -struct commit_list **commit_list_append(struct commit *commit,
> - struct commit_list **next)
> +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));
> + struct replay_insn_list *new = xmalloc(sizeof(struct replay_insn_list));
Tip: we can save the readers some reading and prepare for future
renaming of the structure (not that that's something to fear) using
the idiom
struct replay_insn_list *new = xmalloc(sizeof(*new));
[...]
> -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;
> + struct replay_insn_list *cur = NULL;
> struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
> const char *sha1_abbrev = NULL;
> - const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
> + const char *action_str;
Might be clearer with narrower scope:
struct replay_insn_list *cur;
for (cur = todo_list; cur; cur = cur->next) {
struct commit_message msg = COMMIT_MESSAGE_INIT;
const char *sha1_abbrev, *action_str;
sha1_abbrev = ...;
action_str = ...;
if (get_message(cur->operand, &msg))
return error(...);
strbuf_addf(buf, "%s %s %s\n", ...);
}
return 0;
By the way, shouldn't there a free_message() call to balance out the
get_message()?
[...]
> -static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
> +static int parse_insn_line(char *start, enum replay_action *action,
> + struct commit **operand)
Hm, why not
static int parse_insn_line(const char *start, struct replay_insn_list *item);
> {
> unsigned char commit_sha1[20];
> char sha1_abbrev[40];
> - enum replay_action action;
> - int insn_len = 0;
> + int keyword_len;
What is this renaming about? Maybe "action_len" would work to clarify
that this is about the length of the action string at the start, not
the entire line.
By the way, what are the semantics of that variable? It doesn't seem
to be used, so couldn't we just eliminate it?
> char *p, *q;
>
> if (!prefixcmp(start, "pick ")) {
> - action = REPLAY_PICK;
> - insn_len = strlen("pick");
> - p = start + insn_len + 1;
> + *action = REPLAY_PICK;
> + keyword_len = strlen("pick");
> + p = start + keyword_len + 1;
In such a scenario, this would say
if (!prefixcmp(start, "pick ")) {
item->action = REPLAY_PICK;
p += strlen("pick ");
}
[...]
> } else
> - return NULL;
> + return -1;
Unrelated to this patch: maybe
return error("unrecognized action in sequencer file: %s", start);
to be easier to debug.
> q = strchr(p, ' ');
> if (!q)
> - return NULL;
> + return -1;
So we reject "pick a87c8989"? That's a shame.
> q++;
>
> strlcpy(sha1_abbrev, p, q - p);
memcpy would be clearer. Can't this overflow the sha1_abbrev buffer?
[...]
> if (get_sha1(sha1_abbrev, commit_sha1) < 0)
> - return NULL;
> + return -1;
get_sha1 doesn't print a message, so maybe:
return error("malformed object name in sequencer file: %s",
sha1_abbrev);
> +
> + *operand = lookup_commit_reference(commit_sha1);
> + if (!*operand)
> + return -1;
Perhaps something like
return error("operand %s in sequencer file is not a commit",
sha1_abbrev);
[...]
> @@ -797,18 +791,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 *operand;
Can avoid some churn by keeping the old name here.
> - 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);
> + while ((operand = get_revision(&revs)))
> + next = replay_insn_list_append(opts->action, operand, next);
[...]
> @@ -901,8 +896,9 @@ 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);
> + opts->action = cur->action;
> + res = do_pick_commit(cur->operand, opts);
If do_pick_commit took an "action" operand, this would be less
scary. :)
[...]
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -211,4 +211,62 @@ test_expect_success 'malformed instruction sheet 2' '
> test_must_fail git cherry-pick --continue
> '
>
> +test_expect_success 'revert --continue continues after cherry-pick' '
I haven't looked at the tests yet. FWIW, with whatever changes above
seem suitable,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
^ permalink raw reply
* Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer
From: Jonathan Nieder @ 2011-08-10 15:21 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Git List, Junio C Hamano, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312970151-18906-5-git-send-email-artagnon@gmail.com>
Hi,
Ramkumar Ramachandra wrote:
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -38,6 +38,17 @@ struct replay_insn_list {
> struct replay_insn_list *next;
> };
>
> +/* Unrelated commit_message helper */
> +struct commit_message {
> + char *parent_label;
> + const char *label;
> + const char *subject;
> + char *reencoded_message;
> + const char *message;
> +};
> +
> +int get_commit_message(struct commit *commit, struct commit_message *out);
> +
> /*
> * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
> * any errors. Intended to be used by 'git reset'.
> @@ -48,4 +59,14 @@ struct replay_insn_list {
> */
> void remove_sequencer_state(int aggressive);
>
> +struct replay_insn_list **replay_insn_list_append(enum replay_action action,
> + struct commit *operand,
> + struct replay_insn_list **next);
> +void sequencer_read_todo(struct replay_insn_list **todo_list);
> +void sequencer_read_opts(struct replay_opts **opts_ptr);
> +int sequencer_create_dir(void);
> +void sequencer_save_head(const char *head);
> +void sequencer_save_todo(struct replay_insn_list *todo_list);
> +void sequencer_save_opts(struct replay_opts *opts);
This looks wrong. What is the expected calling sequence? Would it
be possible to expose fewer functions by moving more to sequencer.c?
^ permalink raw reply
* Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer
From: Ramkumar Ramachandra @ 2011-08-10 15:34 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Git List, Junio C Hamano, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <20110810152126.GE31315@elie.gateway.2wire.net>
Hi Jonathan,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> +struct replay_insn_list **replay_insn_list_append(enum replay_action action,
>> + struct commit *operand,
>> + struct replay_insn_list **next);
>> +void sequencer_read_todo(struct replay_insn_list **todo_list);
>> +void sequencer_read_opts(struct replay_opts **opts_ptr);
>> +int sequencer_create_dir(void);
>> +void sequencer_save_head(const char *head);
>> +void sequencer_save_todo(struct replay_insn_list *todo_list);
>> +void sequencer_save_opts(struct replay_opts *opts);
>
> This looks wrong. What is the expected calling sequence? Would it
> be possible to expose fewer functions by moving more to sequencer.c?
Thanks for the early review. Yes, I agree with you: No caller can
make sense of this API; I want something like sequencer_start,
sequencer_handle_conflict, and sequencer_end, but I'm not sure where
to start. I would have liked to just move these functions without
exposing them, but that would break cherry-pick/ revert. So, we're
really faced with two choices:
1. Make this patch enormous by moving as well as refactoring
everything into a beautiful public API. I suspect this won't be easy
to review at all.
2. Keep this patch as it is, and introduce a future patch to clean up
the API. This is the approach I was going for.
I can't move these functions bit-by-bit either: a hypothetical
sequencer_start will require *everything*. Could you suggest
something?
-- Ram
^ permalink raw reply
* Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer
From: Jonathan Nieder @ 2011-08-10 15:53 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Git List, Junio C Hamano, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <CALkWK0nJUTtNgCHF6CE2-w+3ZwdBrscBRw0e0L8wX86Za0G=DA@mail.gmail.com>
Ramkumar Ramachandra wrote:
> So, we're
> really faced with two choices:
> 1. Make this patch enormous by moving as well as refactoring
> everything into a beautiful public API. I suspect this won't be easy
> to review at all.
> 2. Keep this patch as it is, and introduce a future patch to clean up
> the API. This is the approach I was going for.
Well, "beautiful public API" means "just what cmd_cherry_pick and
cmd_revert needs", right? So I'd suggest:
1. Figuring out what functions they need, and doing the minimal
refactoring needed to make them separate functions.
2. As patch #2, moving everything to sequencer.c and exposing those
functions in sequencer.h.
3. In later patches, making changes needed for what "git commit"
needs.
Luckily step (1) is already done. The functions are parse_args()
and pick_revisions() (though they could presumably use less generic
names).
Hmm?
Jonathan
^ permalink raw reply
* Re: git-mergetool: wrap tools with 3 files only to use the BASE file instead of MERGED
From: Jonathan Nieder @ 2011-08-10 16:12 UTC (permalink / raw)
To: Tanguy Ortolo; +Cc: git, David Aguilar, Sebastian Schuberth, Charles Bailey
In-Reply-To: <20110810160356.GA32126@ortolo.eu>
(+cc: some relevant people)
Hi,
Tanguy Ortolo wrote[1]:
> git-mergetool ideally uses tools that work with 4 files: BASE, LOCAL,
> REMOTE, which are the usual original and two new version of the file,
> and MERGED, which is where the tool is supposed to write the result of
> the merge.
>
> The problem is that most tools, at least graphical ones, specifically
> meld, can only work with three files, as they save the result to the
> original file.
>
> git-mergetool currently handles this situation by passing MERGED LOCAL
> REMOTE to the tool. This could be fine, but unfortunately MERGE contains
> the conflicts, formatted for manual resolution, so it is not really
> appropriate as an original file.
>
> I think it would be better to wrap such merge tools by:
> 1. passing them BASE LOCAL REMOTE;
> 2. checking whether or not BASE hase been modified:
> * if it has, then copying it to MERGED,
> * if it has not, exiting with return code 1 (merge failed).
> This check can be by either saving and comparing the mdate, or perhaps
> the SHA-1 hash of the BASE file.
>
> If this sounds good enough, I can dive into git-mergetoo--lib and
> implement it. In the meantime, here is an example of a custom merge tool
> that wraps meld for that purpose.
I think you forgot to include the example. Anyway, at first glance it
sounds like a sensible idea. David et al: thoughts?
Regards,
Jonathan
[1] http://bugs.debian.org/637355
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox