* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Ramkumar Ramachandra @ 2011-08-18 18:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <7v7h6eld2c.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano writes:
> [...]
Here are some comments from my end after extensive thought. Instead
of responding to your comments directly, I've sprinkled bits of it
here and there, reordering it as I see fit. Thanks for detailing out
your thoughts so well :)
0. Before getting into historical mistakes and evaluating existing
workflows, let's first try to detail the problem the sequencer is
trying to solve: pick/ revert aren't the only operations we want to
support. Let's imagine some hypothetical "foo" operation that does
something incredibly complex in the instruction sheet:
pick b38bcc
foo
pick 49ab7c
Now, let's imagine that the "foo" command failed. What is the
strategy we will employ to handle his? I think the question boils
down to: should we teach '--continue' the prerequisite for executing a
"pick" command (clean worktree etc), or how to finish a "foo" command?
What will this answer depend upon?
1. Now, let's discuss the motivation for d3f4628e. I didn't want to
create a separate workflow for pick-one-commit versus
pick-many-commits. It would have to go something like:
parse_arguments(whatever tree'ishes specified on argc)
setup_revision_walker
n = count_revisions
if (n == 1)
do foo; // Don't create sequencer state
else
create sequencer state; do foo; cleanup sequencer state;
Ugly. Wouldn't you agree? So, what choice did I have? I can't
conditionally create sequencer state. So, an idea struck me: if I
clean it up before someone notices, I'm safe. So, I decided to remove
it before the last commit -- when there's only one commit, it'll be
removed immediately. So far so good: no regressions introduced.
2. Okay, now let's try to touch upon the issue of marking something as "done":
> - The case to resume an unexpected stop (i.e. "pick" that causes conflict
> or "rebase" without "-i") also feels right. The user fixes up and marks
> things as "done" with "add/rm", and tell the stopped command that it can
> now continue with "rebase --continue". The same comment applies for
> "am".
I could argue that "add/rm" need not be the only way to specify "done"
in the general case. We needn't be consistent about that- we can
print hints about what "done" would mean, and not invent a new
"--mark-as-done" command. What varies here is the definition of
"done": does it mean that the user has "done" what the instruction
sheet operation was supposed to do (including the committing), or has
simply done a part of the job and passed on the baton?
3. How tight do we want the sequencer to be? Should we allow
executing other commands during the operation of the sequencer, or
should we simply expect the user to "--continue" the operation first
and take care of everything else later?
> - The sequencing flow the current "rebase -i" implements when resuming a
> controlled stop (i.e. "edit" or "reword"), even as the last step of the
> insn sheet, feels like the right thing. The actual tweaking of the
> commit done by "commit --amend" is oblivious to the sequencing state,
> and resuming is controlled by "rebase --continue".
I often "git commit -m temp" in the middle of a "rebase -i", and I'm
strongly in favor of allowing this somehow.
> There is a (complicated) workflow to split an commit during
> "rebase -i" that does _not_ want an auto-resume by a commit.
Exactly! :)
Auto-resuming after a "commit" is fundamentally wrong.
> Also not
> all conflicted/stopped state can be concluded with "commit" (think:
> "rebase/am --skip").
A good point on consistency.
4. Let's correct the historical mistakes and get our act together.
The final pending question is: how many historical mistakes are we
willing to stomp now?
> I personally am leaning towards teaching "--continue" to "merge" and
> "cherry-pick", while keeping only existing awareness of these two commands
> in "commit" as historical mistakes (look for 'unlink(git_path("[A-Z_]*"))'
> in builtin/commit.c). That would mean that in the long run, new users need
> to learn only one thing: when "git Foo" stops because it needs help from
> you resolving conflicts, after you help it by editing the files in your
> working tree and making that with add/rm, you say "git Foo --continue" to
> signal that you are done helping it.
Okay, stomp nothing. Under this constraint, the best we can do is:
1. Introduce a 'merge --continue' to invoke 'git commit'. MERGE_HEAD
helps 'git commit' finish. Modify tests to use '--continue' instead
of the earlier commit-to-finish workflow, and advertise this feature
everywhere.
2. Make 'cherry-pick --continue' invoke 'git commit' as well.
CHERRY_PICK_HEAD helps 'git commit' finish. If the commit finishes
successfully: (if there is one commit left, remove the sequencer
state; otherwise, drop the first insn on top of the list and execute
the next insn). Modify tests to use '--continue' instead of the
earlier commit-to-finish workflow, and advertise this feature
everywhere. Unfortunately, if the user executes 'git commit' instead
of the newer '--continue', we're screwed: a stray sequencer state will
be left behind.
Does the above seem sensible, or should we do something about the
historical mistakes?
Thanks.
-- Ram
^ permalink raw reply
* [GSoC update] Sequencer: Dealing with historical mistakes
From: Ramkumar Ramachandra @ 2011-08-18 19:15 UTC (permalink / raw)
To: Git List
Cc: Junio C Hamano, Jonathan Nieder, Daniel Barkalow,
Christian Couder, Jeff King
Hi,
Junio suddenly raised some very disturbing concerns about the overall
UI consistency wrt '--continue'. I'm really glad he raised it before
we locked ourselves into yet another UI mistake. Work on the
generalized sequencer series has totally halted since, and I've been
pondering about what to do. After some thought, I've tried to
untangle the various issues and add something useful to the discussion
[1]. I'm somewhat annoyed that we're stuck with these historical
mistakes, and I hope that our future design isn't constrained too
badly.
Otherwise, I've requested Junio to merge 'rr/cherry-pick-continue'
as-it-is, since it doesn't have any major issues. The generalized
sequencer series itself will take some more time- we have to decide
the final implementation details at the end of the discussion. Also,
the GSoC is drawing to a close, and I plan to post a somewhat
elaborate final update. If there's something in particular you'd like
to see in it, do let me know.
Thanks for reading.
[1]: http://mid.gmane.org/CALkWK0noHBnW-7zZLw=jJdDVFxXmsm2vHHYnUJc9miLLuDRnAg@mail.gmail.com
-- Ram
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Jonathan Nieder @ 2011-08-18 19:18 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <CALkWK0noHBnW-7zZLw=jJdDVFxXmsm2vHHYnUJc9miLLuDRnAg@mail.gmail.com>
Hi Ram,
Ramkumar Ramachandra wrote:
> Here are some comments from my end after extensive thought.
Could be briefer. :)
[...]
> 1. Introduce a 'merge --continue' to invoke 'git commit'. MERGE_HEAD
> helps 'git commit' finish. Modify tests to use '--continue' instead
> of the earlier commit-to-finish workflow, and advertise this feature
> everywhere.
Why modify tests? I think "git merge --continue" is a nice idea,
and I don't see how it's inconsistent in any way with continuing to
allow old practice.
> 2. Make 'cherry-pick --continue' invoke 'git commit' as well.
> CHERRY_PICK_HEAD helps 'git commit' finish. If the commit finishes
> successfully: (if there is one commit left, remove the sequencer
> state; otherwise, drop the first insn on top of the list and execute
> the next insn).
Sounds like a sensible thing to do. I assume the "one" in the
parenthesis is supposed to be "zero", making the "if" not even part of
the user-visible description of what it does --- it's just the
termination condition of a loop.
"git cherry-pick --continue" in place of "git commit" does not handle
the following scenario. Suppose my multiple-cherry-pick has run into
conflicts, and while fixing them I notice something related that needs
to be fixed.
... resolve conflict, leaving extra change in worktree ...
git stash -k
... test test test ...
git commit
git stash pop
git commit; # make a separate commit for extra change
# ok, now continue.
git sequencer --continue
In other words, in this sequence of commands, "git commit" is used to
single-step. So if one wants to remove CHERRY_PICK_HEAD altogether, a
nice thing to do would be to introduce a "git sequencer --single-step"
command or something similar to handle such cases.
> Modify tests to use '--continue' instead of the
> earlier commit-to-finish workflow, and advertise this feature
> everywhere. Unfortunately, if the user executes 'git commit' instead
> of the newer '--continue', we're screwed: a stray sequencer state will
> be left behind.
As Junio hinted, it could make a lot of sense for "git cherry-pick
<single commit>" to not create sequencer state in the first place.
"git cherry-pick --continue" does not need it --- it is enough to
commit with the conflict resolved. "git cherry-pick --abort" does not
need it, either --- it is enough to "git reset --merge HEAD".
One part I'm handwaving is what to do about commands like "git
cherry-pick foo^..foo" which use a commit range that only happens to
contain one commit. Either behavior seems fine for such commands.
What do you think?
^ permalink raw reply
* Re: [PATCH v4 2/2] push: Don't push a repository with unpushed submodules
From: Heiko Voigt @ 2011-08-18 19:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Fredrik Gustafsson, git, jens.lehmann
In-Reply-To: <1313312613-2647-1-git-send-email-iveqy@iveqy.com>
Hi Junio,
On Sun, Aug 14, 2011 at 11:03:33AM +0200, Fredrik Gustafsson wrote:
> 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.
Since I do not see thisi in pu maybe you missed this? There was a series
containing the same patch send before which Jens asked to ignore but
this was the one which was meant to be included.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Ramkumar Ramachandra @ 2011-08-18 19:50 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <20110818191812.GG30436@elie.gateway.2wire.net>
Hi Jonathan,
Jonathan Nieder writes:
> Could be briefer. :)
Sorry about the braindump :P
>> 1. Introduce a 'merge --continue' to invoke 'git commit'. MERGE_HEAD
>> helps 'git commit' finish. Modify tests to use '--continue' instead
>> of the earlier commit-to-finish workflow, and advertise this feature
>> everywhere.
>
> Why modify tests? I think "git merge --continue" is a nice idea,
> and I don't see how it's inconsistent in any way with continuing to
> allow old practice.
In the future, we might want a 'merge' instruction in the sequencer --
I want to make it clear that we're going for a significant UI change
so that everyone (including tests, scripts) become comfortable with
the new UI.
>> 2. Make 'cherry-pick --continue' invoke 'git commit' as well.
>> CHERRY_PICK_HEAD helps 'git commit' finish. If the commit finishes
>> successfully: (if there is one commit left, remove the sequencer
>> state; otherwise, drop the first insn on top of the list and execute
>> the next insn).
>
> Sounds like a sensible thing to do. I assume the "one" in the
> parenthesis is supposed to be "zero", making the "if" not even part of
> the user-visible description of what it does --- it's just the
> termination condition of a loop.
Right, sorry about the convoluted thought.
> As Junio hinted, it could make a lot of sense for "git cherry-pick
> <single commit>" to not create sequencer state in the first place.
> "git cherry-pick --continue" does not need it --- it is enough to
> commit with the conflict resolved. "git cherry-pick --abort" does not
> need it, either --- it is enough to "git reset --merge HEAD".
Okay, here's my problem with the idea: it'll essentially require the
sequencer to differentiate between one-commit operations and
many-commit operations. In the case of one-commit operations, *every*
new command that calls into the sequencer will will need to persist
information in its own way using hacks like CHERRY_PICK_HEAD and
MERGE_HEAD. And we have to make "git commit" unlink yet another file
:) I'm not talking about some hypothetical case: I'm already planning
to make 'git am' call into the sequencer, so we'll need an AM_HEAD.
One final resort: Move some code back into cherry-pick, and call into
a later-function in the sequencer only if it's a many-commit
operation. The new commands can enjoy the comfort of calling into an
earlier-function in the sequencer that'll do all the revision walk
setup and call the later-function. I think this is reasonable.
> One part I'm handwaving is what to do about commands like "git
> cherry-pick foo^..foo" which use a commit range that only happens to
> contain one commit. Either behavior seems fine for such commands.
I don't think I follow. This will be determined as a single-commit
operation after setting up the revisions. I don't think it should be
treated as a multi-commit operation because the literal tree'ish
contains "..".
-- Ram
^ permalink raw reply
* Re: [PATCH 1/5] decoration: do not mis-decorate refs with same prefix
From: Junio C Hamano @ 2011-08-18 17:58 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1313670578-2939-2-git-send-email-pclouds@gmail.com>
Good eyes; thanks.
^ permalink raw reply
* Re: [PATCH 3/5] Move write_shallow_commits to fetch-pack.c
From: Junio C Hamano @ 2011-08-18 18:01 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1313670578-2939-4-git-send-email-pclouds@gmail.com>
Nice; thanks.
^ permalink raw reply
* Re: [PATCH 5/5] log: decorate "replaced" on to replaced commits
From: Junio C Hamano @ 2011-08-18 18:16 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1313670578-2939-6-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Old code also decorates "new" commits with "refs/replace/SHA1". This
> is now gone, but I guess no one will miss it.
Makes sense but...
> + if (!prefixcmp(refname, "refs/replace/")) {
> + unsigned char original_sha1[20];
> + if (get_sha1_hex(refname + 13, original_sha1)) {
> + warning("invalid replace ref %s", refname);
> + return 0;
> + }
> + obj = parse_object(original_sha1);
> + if (obj && (obj->type == OBJ_COMMIT || obj->type == OBJ_TAG))
... is it necessary to check and limit the types like this here?
If the argument is "we know only commits and tags are listed and blobs and
trees are not shown with --decorate option" and "excluding the decoration
that we know will never be used will avoid bloating the decorate hashtable
with unused cruft", then add_name_decoration() should be doing the check
for all of its callers, not just this one, no?
> + add_name_decoration(DECORATION_GRAFTED, "replaced", obj);
> + return 0;
> + }
> +
> + obj = parse_object(sha1);
> if (!obj)
> return 0;
^ permalink raw reply
* Re: [PATCH v3] Accept tags in HEAD or MERGE_HEAD
From: Junio C Hamano @ 2011-08-18 18:54 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1313674994-22902-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> HEAD and MERGE_HEAD (among other branch tips) should never hold a
> tag. That can only be caused by broken tools and is cumbersome to fix
> by an end user with:
>
> $ git update-ref HEAD $(git rev-parse HEAD^{commit})
>
> which may look like a magic to a new person.
>
> Be easy, warn users (so broken tools can be fixed) and move on.
We do not really fix broken tools; we just fix breakages caused by them.
> Be robust, if the given SHA-1 cannot be resolved to a commit object,
> die (therefore return value is always valid).
Makes sense.
> diff --git a/builtin/commit.c b/builtin/commit.c
> index cb73857..f78b449 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -63,6 +63,7 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
> "Otherwise, please use 'git reset'\n");
>
> static unsigned char head_sha1[20];
> +static struct commit *head_commit;
I was not happy with the file-scope global head_sha1[] already, and this
makes me even less happy. Was it too much trouble to keep them local to
cmd_commit() and pass them around as arguments where necessary? If you
pass around head_commit, is_null_sha1(head_sha1) can be replaced with a
check !head_commit so we may even be able to lose the head_sha1[] global.
And actually removing head_sha1[] is a necessary step from the correctness
point of view. The repository may have given an object name for a tag in
head_sha1[] and lookup_expect_commit() may have peeled it to a commit.
The code may want to add the "HEAD" as one of the parents of a new commit,
and head_sha1[] is no longer the place to pick that information up after
your fix. The code needs to look at head_commit->object.sha1[] instead.
Losing the use of head_sha1[] variable and forcing the code to go to
head_commit->object.sha1[] reduces the risk of such confusion.
> @@ -1028,6 +1026,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
>
> if (get_sha1("HEAD", head_sha1))
> initial_commit = 1;
> + else
> + head_commit = lookup_expect_commit(head_sha1, "HEAD");
It may be just me, but the name feels a bit funny. The original name was
"look up" (verb) + "commit" (direct object of the verb), and what you are
doing is lookup_and_expect_commit(), but it is too long.
Perhaps lookup_commit_or_die()?
To a naïve reader of the caller, the function looks as if it would do
struct commit *lookup_expect_commit(unsigned char *head_sha1, char *name)
{
struct commit *c;
if (get_sha1("HEAD", head_sha1) ||
!(c = lookup_commit(head_sha1))) die(...);
return c;
}
but in fact it does not use "HEAD" for anything other than error reporting
and it is the caller's responsibility to supply head_sha1[].
It is documented in the function declaration by making the memory
head_sha1 points at as a "const" (i.e. the function can't be calling
get_sha() on the refname), but it may also need a comment or two there.
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 325891e..22e98e8 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -51,6 +51,7 @@ static int allow_trivial = 1, have_message;
> static struct strbuf merge_msg;
> static struct commit_list *remoteheads;
> static unsigned char head[20], stash[20];
> +static struct commit* head_commit;
This is C, not C++; asterisk sticks to the variable, not type.
> @@ -1030,6 +1031,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> branch += 11;
> if (is_null_sha1(head))
> head_invalid = 1;
> + else
> + head_commit = lookup_expect_commit(head, "HEAD");
>
> git_config(git_merge_config, NULL);
>
The same comment as head[] vs head_commit->object.sha1[] redundancy
applies from builtin/commit.c here.
Overall, the changes to buitlin/merge.c look very nice, getting rid of the
repeated lookup_commit(head). In a separate patch after this fix gets
ready, we may want to further clean it up so that head_commit being NULL
means head_invalid, losing a redundant variable.
^ permalink raw reply
* Re: [PATCH 4/5] log: decorate grafted commits with "grafted"
From: Junio C Hamano @ 2011-08-18 18:10 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1313670578-2939-5-git-send-email-pclouds@gmail.com>
I think this makes a lot more sense than the other option of looking up
graft points every time, as we expect only a handful of grafts, if any,
exist in a repository.
If you have, on the other hand, tons of historical graft points, and you
are viewing only near the tip (e.g. "log -20"), this implimentation still
calls lookup_commit() for all those historical graft points that are
likely to be scattered further back in the packfile, and the performance
may suffer. But that is what it means when we say that we are optimized
for use case where only a handful of grafts exist, so it is not something
that we worry about too much.
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Jonathan Nieder @ 2011-08-18 20:05 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <CALkWK0=jRAq6s1zQ5gwB4feBgC1eo=VYLWx8bsjs+exqmz0f1A@mail.gmail.com>
Ramkumar Ramachandra wrote:
> In the future, we might want a 'merge' instruction in the sequencer --
> I want to make it clear that we're going for a significant UI change
> so that everyone (including tests, scripts) become comfortable with
> the new UI.
I don't follow. I meant "Why modify tests, rather than adding new
ones?" Tests document what is supposed to continue working.
[...]
> Okay, here's my problem with the idea: it'll essentially require the
> sequencer to differentiate between one-commit operations and
> many-commit operations. In the case of one-commit operations, *every*
> new command that calls into the sequencer will will need to persist
> information in its own way using hacks like CHERRY_PICK_HEAD and
> MERGE_HEAD.
Why wouldn't such a backward-compatibility feature apply to
cherry-pick/revert only?
> I'm not talking about some hypothetical case: I'm already planning
> to make 'git am' call into the sequencer, so we'll need an AM_HEAD.
Yuck. How does the UI distinguish between
git sequencer --continue; # apply the rest of the patches from mbox
and
git sequencer --continue; # finish up "am" insn and continue
? Does the sequencer need an argument to indicate the level of
nesting? I would find it more straightforward for "git am mbox" to
create a todo list saying something like
apply patch 1 from mbox
apply patch 2 from mbox
apply patch 3 from mbox
apply patch 4 from mbox
so there would still be only one pending sequence of basic
instructions to think about.
> One final resort: Move some code back into cherry-pick, and call into
> a later-function in the sequencer only if it's a many-commit
> operation. The new commands can enjoy the comfort of calling into an
> earlier-function in the sequencer that'll do all the revision walk
> setup and call the later-function. I think this is reasonable.
I'm having trouble parsing this; sorry. What is the effect on the
user-visible behavior of the program of what you propose?
> Jonathan Nieder writes:
>> One part I'm handwaving is what to do about commands like "git
>> cherry-pick foo^..foo" which use a commit range that only happens to
>> contain one commit. Either behavior seems fine for such commands.
>
> I don't think I follow. This will be determined as a single-commit
> operation after setting up the revisions. I don't think it should be
> treated as a multi-commit operation because the literal tree'ish
> contains "..".
I said "Either behavior seems fine", didn't I?
Hope that clarifies a little.
Jonathan
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Ramkumar Ramachandra @ 2011-08-18 20:06 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <CALkWK0=jRAq6s1zQ5gwB4feBgC1eo=VYLWx8bsjs+exqmz0f1A@mail.gmail.com>
HI again,
Ramkumar Ramachandra writes:
> One final resort: Move some code back into cherry-pick, and call into
> a later-function in the sequencer only if it's a many-commit
> operation. The new commands can enjoy the comfort of calling into an
> earlier-function in the sequencer that'll do all the revision walk
> setup and call the later-function. I think this is reasonable.
Er, no. This would be amazingly ugly too: we'd need two versions of
everything like '--continue'. And look at this glaring inconsistency:
git cherry-pick foo..bar
..conflict..
git sequencer --abort
$? = 0
But:
git cherry-pick foo
..conflict..
git sequencer --abort
error: No sequencer operation in progress.
In a nutshell, the problem we're facing: We persist part of the
continuation data outside the sequencer's knowledge, and part of it
inside. For backward compatibility, we can't move the former part
into the sequencer.
-- Ram
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Jonathan Nieder @ 2011-08-18 20:12 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Junio C Hamano, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <CALkWK0nV35gNFM2NW-FrwBLnvFzmWDdARZk0Xy6LT8c2UE0fAw@mail.gmail.com>
Ramkumar Ramachandra wrote:
> But:
>
> git cherry-pick foo
> ..conflict..
> git sequencer --abort
> error: No sequencer operation in progress.
I mentioned this before: "git sequencer --abort" could notice that
CHERRY_PICK_HEAD exists and .git/sequencer does not and act
accordingly.
> In a nutshell, the problem we're facing: We persist part of the
> continuation data outside the sequencer's knowledge, and part of it
> inside. For backward compatibility, we can't move the former part
> into the sequencer.
Why can't it become the sequencer's responsibility, FWIW? That's an
implementation detail.
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Junio C Hamano @ 2011-08-18 20:22 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Jonathan Nieder, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <CALkWK0=jRAq6s1zQ5gwB4feBgC1eo=VYLWx8bsjs+exqmz0f1A@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> Why modify tests? I think "git merge --continue" is a nice idea,
>> and I don't see how it's inconsistent in any way with continuing to
>> allow old practice.
I agree. Updating the test will hide a regression if Ram's update breaks
the existing workflow to conclude a conflicted merge with "git commit".
If we are to add "git merge --continue" sugarcoat to make it easier to
teach new people saying that "To any Git operation that stops and asks you
to help, you can tell it that you are done helping by re-running the same
command with --continue flag", then _new_ tests should be added to make
sure that "git merge --continue" does act just like "git commit" in such a
case.
> In the future, we might want a 'merge' instruction in the sequencer --
The end-user facing frontend may be "git rebase" in such a case, and we
would be teaching the users "When you are done helping the conflicted
'rebase', tell it that you are done by running 'rebase --continue'". The
command verb 'merge' on the sequencer insn sheet does not have any direct
connection to 'git merge' command (e.g. 'pick' does not have to be and is
not implemented using 'git pick' command that does not exist). So I do not
think we would ever say "merge --continue" in this context to begin with.
>> As Junio hinted, it could make a lot of sense for "git cherry-pick
>> <single commit>" to not create sequencer state in the first place.
>> "git cherry-pick --continue" does not need it --- it is enough to
>> commit with the conflict resolved. "git cherry-pick --abort" does not
>> need it, either --- it is enough to "git reset --merge HEAD".
>
> Okay, here's my problem with the idea: it'll essentially require the
> sequencer to differentiate between one-commit operations and
> many-commit operations.
I think you are looking at it in a wrong way. It is just about giving
backward compatibility to historical hacks. cherry-pick and revert may be
the only ones needed, and a new command Foo that implements its workflow
in terms of the sequencer can choose to (and should choose to unless there
is a compelling reason not to, because of the exact reason you stated
here) do a single-command insn sheet with "git Foo --continue" to conclude
if that one and only step needed help from the end user.
"git am" would fit in the picture the same way. The sequencer insn sheet
it would produce would consist of "patch <filename>" or something, and
after the last one of them fails, the user would fix things up in the
working tree, "git am --continue" will create a commit, and because there
is no more steps in the sequence, the sequencer state will be removed.
^ permalink raw reply
* Re: [PATCH v4 2/2] push: Don't push a repository with unpushed submodules
From: Junio C Hamano @ 2011-08-18 20:32 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Fredrik Gustafsson, git, jens.lehmann
In-Reply-To: <20110818192157.GA10598@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> On Sun, Aug 14, 2011 at 11:03:33AM +0200, Fredrik Gustafsson wrote:
>> 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.
>
> Since I do not see thisi in pu maybe you missed this? There was a series
> containing the same patch send before which Jens asked to ignore but
> this was the one which was meant to be included.
Hmm, this is how the messages look like in my threaded MUA:
[ 34: Fredrik Gustafsson ] [PATCH v4 0/2] push limitations
[ 377: Fredrik Gustafsson ] [PATCH v4 2/2] push: Don't push a reposi...
[ 35: Jens Lehmann ] Re: [PATCH v4 0/2] push limitations
[ 377: Fredrik Gustafsson ] [PATCH v4 2/2] push: Don't push a reposit...
[ 14: Heiko Voigt ]
where Jens says "Please ignore this series, it slipped by accident" in the
third one. And the second [PATCH v4 2/2] message that you are reminding me
of (thanks!) has the identical proposed commit log message and patch text
as the other [PATCH v4 2/2] message.
So I am not sure what is going on here.
Also I vaguely recall that I said something about the command line parser
for this new option during the review of the previous round. Has that been
resolved/corrected?
^ permalink raw reply
* Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit
From: Junio C Hamano @ 2011-08-18 20:42 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Ramkumar Ramachandra, Git List, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <20110818191812.GG30436@elie.gateway.2wire.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> One part I'm handwaving is what to do about commands like "git
> cherry-pick foo^..foo" which use a commit range that only happens to
> contain one commit.
Historically cherry-pick took a single commit from the command line. That
can easily be distinguished from a "set that ended up with one positive
commit after walking the history". I think you can even check before
triggering the revision walking machinery in the first place.
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2011, #03; Thu, 11)
From: Pascal Obry @ 2011-08-18 21:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vr54rpogf.fsf@alter.siamese.dyndns.org>
Junio,
> * po/cygwin-backslash (2011-08-05) 2 commits
> - On Cygwin support both UNIX and DOS style path-names
> - git-compat-util: add generic find_last_dir_sep that respects is_dir_sep
>
> I think a further refactoring (no, not my suggestion) was offered?
I think the current patchset is fine. It is always possible to improve
things but the current patch goes in the right direction. So to me it is
ready as-is.
Pascal.
--
--|------------------------------------------------------
--| 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] fetch-pack: check for valid commit from server
From: Shawn Pearce @ 2011-08-18 21:32 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1313674563-12755-1-git-send-email-pclouds@gmail.com>
2011/8/18 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> A malicious server can return ACK with non-existent SHA-1 or not a
> commit. lookup_commit() in this case may return NULL. Do not let
> fetch-pack crash by accessing NULL address in this case.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> However it raises another question, what if the other end returns a
> valid commit, but not the one in "have" line fetch-pack sent? Are we
> OK with that?
Not really. The server is not supposed to return a SHA-1 in the ACK
line unless the client said it first in a have line. So aborting with
an error is reasonable thing for a client to do.
> + if (!commit)
> + die("invalid commit %s", sha1_to_hex(result_sha1));
Maybe:
die("server ACK contained unknown commit %s", sha1_to_hex(result_sha1));
is more specific to the problem.
Just curious, did you see this on a particular server somewhere?
--
Shawn.
^ permalink raw reply
* Re: [PATCH 1/4] git-p4: Allow setting rename/copy detection threshold.
From: Vitor Antunes @ 2011-08-18 21:50 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, Tor Arvid Lund
In-Reply-To: <20110818151847.GA2043@padd.com>
On Thu, Aug 18, 2011 at 4:18 PM, Pete Wyckoff <pw@padd.com> wrote:
> This is a good idea. But I notice that -M, -C, -B can take
> a diverse set of option forms, including floating point and
> optional "%". "-B" can take two args, separated by a "/".
>
> See diff_scoreopt_parse(). It would be weird for git-p4 to limit
> that set to integers like you do here.
>
> Would you be willing to change it to pass the string blindly?
> Just filter out the special values, and pass everything else
> unchanged.
>
> "true" -> "-M"
> "false" -> ""
> "" -> ""
> ".*" -> "\1"
This makes sense. I did notice that -M and -C supported, at least, a
'%' char at the end. But I decided to ignore it. I think your is idea
is not only good, but also a simpler solution. I'll send a new set of
patches shortly with this implemented.
> Dunno if you care about --break-rewrites. Just pointing to it
> as a similar option to -M and -C in case you missed it.
I was completely unaware of this option, so I don't feel comfortable
in adding it as a configuration option for now. Will need to
understand it first ;)
--
Vitor Antunes
^ permalink raw reply
* Re: [PATCH 0/10] color and pager improvements
From: Jeff King @ 2011-08-18 21:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110818045821.GA17377@sigill.intra.peff.net>
On Wed, Aug 17, 2011 at 09:58:23PM -0700, Jeff King wrote:
> While looking at the pager and color code today, I decided to tackle two
> long-standing bugs, which entailed a lot of refactoring of the color
> code. The result fixes some minor bugs, and is a little nicer for
> calling code to use.
And here are two patches on top of the previous 10 to help Ingo's
problem a bit.
[11/10]: support pager.* for aliases
[12/10]: support pager.* for external commands
With these, you can do "git config pager.stash false" to turn off paging
on "stash list" and "stash show" (and turn it back on with "git -p
stash list", of course).
I'm slightly tempted to allow things like "pager.stash.list" and
"pager.branch.list". It wouldn't be too hard to implement. But I'm not
sure anybody actually cares. I think Ingo's original complaint was
simply that pager.stash didn't actually do anything, not that he wanted
some separate config for the various subcommands.
-Peff
^ permalink raw reply
* [PATCH 11/10] support pager.* for aliases
From: Jeff King @ 2011-08-18 21:59 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110818215820.GA7767@sigill.intra.peff.net>
Until this patch, doing something like:
git config alias.foo log
git config pager.foo /some/specific/pager
would not respect pager.foo at all. With this patch, we
will use pager.foo for the "foo" alias. We will also
fallback to pager.log if "foo" is a non-shell alias that
uses the "log" command (but any pager.foo overrides
pager.log).
Signed-off-by: Jeff King <peff@peff.net>
---
git.c | 3 +++
| 31 +++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/git.c b/git.c
index 8828c18..375e9b2 100644
--- a/git.c
+++ b/git.c
@@ -180,6 +180,9 @@ static int handle_alias(int *argcp, const char ***argv)
alias_command = (*argv)[0];
alias_string = alias_lookup(alias_command);
if (alias_string) {
+ if (use_pager == -1)
+ use_pager = check_pager_config(alias_command);
+
if (alias_string[0] == '!') {
const char **alias_argv;
int argc = *argcp, i;
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4582336..a8c6e85 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -450,4 +450,35 @@ test_expect_success TTY 'command-specific pager overridden by environment' '
test_cmp expect actual
'
+test_expect_success TTY 'command-specific pager works for aliases' '
+ sane_unset PAGER GIT_PAGER &&
+ echo "foo:initial" >expect &&
+ >actual &&
+ test_config alias.aliaslog "log --format=%s" &&
+ test_config pager.aliaslog "sed s/^/foo:/ >actual" &&
+ test_terminal git aliaslog -1 &&
+ test_cmp expect actual
+'
+
+test_expect_success TTY 'non-shell alias falls back to command pager config' '
+ sane_unset PAGER GIT_PAGER &&
+ echo "foo:initial" >expect &&
+ >actual &&
+ test_config alias.aliaslog "log --format=%s" &&
+ test_config pager.log "sed s/^/foo:/ >actual" &&
+ test_terminal git aliaslog -1 &&
+ test_cmp expect actual
+'
+
+test_expect_success TTY 'alias-specific pager can override aliased command' '
+ sane_unset PAGER GIT_PAGER &&
+ >expect &&
+ >actual &&
+ test_config alias.aliaslog "log --format=%s" &&
+ test_config pager.log "sed s/^/log:/ >actual" &&
+ test_config pager.aliaslog false &&
+ test_terminal git aliaslog -1 &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.6.10.g62f04
^ permalink raw reply related
* Re: [PATCH 0/10] color and pager improvements
From: Steffen Daode Nurpmeso @ 2011-08-18 21:59 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20110818045821.GA17377@sigill.intra.peff.net>
@ Jeff King <peff@peff.net> wrote (2011-08-18 06:58+0200):
> These three fix the problem Steffen mentioned here:
Uuuh, such a shame - you know that it was first noted by
Benjamin Kudria (2008-07-23,
http://marc.info/?l=git&m=121677902502581&w=2).
And it was you who tried to resurrect the same issue last year.
(The thing is: i did not search the archive first because it was
clearly a bug. I did once you referred to your older patch.)
But great that you actually found the time to fix it!
(I must admit though that i'm currently addicted to the coloured
output, because simply switching between my terms gives a clear
indication of where i'm currently git(1)ing. :-> And that in
turn is something which gives more and more fun the longer i use
it! It is *really* fantastic once you get used to it. And do
gc --aggressive and all your temporary fooling is cleaned up.)
Now it's pretty unfortunate that i cannot offer fixes for
anything.
I have a dumb patch of 'rebase -i' which includes the TODO entry
line ($rest) as a comment in the commit message, which is pretty
useful because i think about the rebase task and can store
comments in that very line. But it introduces commit
--cleanup=strip and patches commit.c to add a --message-prefix
option. This is no good yet.
Michael J Gruber's today's shocking exercise on the german
keyboard layout - maybe i should really resurrect parts of that
NBSP series?
And referring to one sentence of yours from the past: no, refspec
stuff *is* that hard: they are not a tree which is created via
'refs_build_tree(); refs_merge_command_line();' upon program
start, with pointers to maybe instantiated .. whatever.
/*
* Note. This is used only by "push"; refspec matching rules for
* push and fetch are subtly different, so do not try to reuse it
* without thinking.
*/
I gave up once i found that (in remote.c). (AFAIR it seems
refspecs are first build L->R, then pushed, then build again but
in R->L direction. Which is why without a fetch= the remotes/
ref is not updated after a push. AFAIK - i gave up ...)
But i'm looking forward and really hope to be able to contribute
some useful and good stuff to great projects in the future.
OpenBSD, for example. :-)
--Steffen
Ciao, sdaoden(*)(gmail.com)
ASCII ribbon campaign ( ) More nuclear fission plants
against HTML e-mail X can serve more coloured
and proprietary attachments / \ and sounding animations
^ permalink raw reply
* update-index --index-info producing spurious submodule commits
From: Greg Troxel @ 2011-08-18 21:53 UTC (permalink / raw)
To: git; +Cc: Richard Hansen
[-- Attachment #1.1: Type: text/plain, Size: 2624 bytes --]
For reasons too complicated to go into, I have a repository B which has
essentially been cloned from A, and there has been vast amounts of work
on B (thousands of commits, many branches). These changes have not
been merged back to A. I want to merge them back, but there's a
directory foo that has changes in B that I can't release.
So, I ran filter-branch with an index filter
found the merge base with A
removed foo
did ls-tree on foo from merge base
and updated the index
The theory is to make each commit in B look like no changes to anything
under foo, and otherwise the same.
After doing this, Richard noticed that the root tree of commits had a
foo object, but that it was labeled a commit instead of a tree (but in
fact it is a tree). He noticed because diffs looked like submodules.
I was able to produce a minimal test case, output below, script
attached. The below output is with 1.7.5.4 on NetBSD/i386 (and /amd64).
1.7.6 (ubuntu/amd64) has the same problem.
So:
Am I using "git update-index --index-info" wrong?
Or is there a bug?
Thanks,
Greg
Notice that "cat-file -p HEAD:" shows a tree before, and a commit
afterwards:
+ git init
Initialized empty Git repository in /usr/home/gdt/GIT_TEST/.git/
+ mkdir foo
+ touch foo/bar
+ git add foo
+ git commit -minitial content
[master (root-commit) 6755919] initial content
0 files changed, 0 insertions(+), 0 deletions(-)
create mode 100644 foo/bar
+ git cat-file -p HEAD
tree 72d67e6de0599f72f1265c925316f91f78395787
author Greg Troxel <gdt@ir.bbn.com> 1313703545 -0400
committer Greg Troxel <gdt@ir.bbn.com> 1313703545 -0400
initial content
+ git cat-file -p HEAD:
040000 tree d87cbcba0e2ede0752bdafc5938da35546803ba5 foo
+ git rm -r foo
rm 'foo/bar'
+ git ls-tree HEAD foo
040000 tree d87cbcba0e2ede0752bdafc5938da35546803ba5 foo
+ git ls-tree HEAD foo
+ git update-index --index-info
+ git diff --staged
diff --git a/foo b/foo
new file mode 160000
index 0000000..d87cbcb
--- /dev/null
+++ b/foo
@@ -0,0 +1 @@
+Subproject commit d87cbcba0e2ede0752bdafc5938da35546803ba5
diff --git a/foo/bar b/foo/bar
deleted file mode 100644
index e69de29..0000000
+ git commit -mmunged foo
[master 3348447] munged foo
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 160000 foo
delete mode 100644 foo/bar
+ git cat-file -p HEAD
tree 04fbd499dbd01afb3241d7f0af8171fde008bfe3
parent 6755919e289665ec46d270672d29b594f992fa03
author Greg Troxel <gdt@ir.bbn.com> 1313703545 -0400
committer Greg Troxel <gdt@ir.bbn.com> 1313703545 -0400
munged foo
+ git cat-file -p HEAD:
160000 commit d87cbcba0e2ede0752bdafc5938da35546803ba5 foo
[-- Attachment #1.2: git-example --]
[-- Type: text/plain, Size: 372 bytes --]
#!/bin/sh
if [ -d .git ]; then
echo "existing .git"
exit 1
fi
set -x
git init
mkdir foo
touch foo/bar
git add foo
git commit -m'initial content'
git cat-file -p HEAD
git cat-file -p HEAD:
git rm -r foo
git ls-tree HEAD foo
git ls-tree HEAD foo | git update-index --index-info
git diff --staged
git commit -m'munged foo'
git cat-file -p HEAD
git cat-file -p HEAD:
[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]
^ permalink raw reply related
* [PATCH 12/10] support pager.* for external commands
From: Jeff King @ 2011-08-18 22:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110818215820.GA7767@sigill.intra.peff.net>
Without this patch, any commands that are not builtin would
not respect pager.* config. For example:
git config pager.stash false
git stash list
would still use a pager. With this patch, pager.stash now
has an effect. If it is not specified, we will still fall
back to pager.log when we invoke "log" from "stash list".
Signed-off-by: Jeff King <peff@peff.net>
---
I think we didn't do this in the original pager.* patches because of
initialization order problems. It was dangerous to look at config too
early in the process, or something similar; I don't recall the exact
problems. But since work from Jonathan and Duy last summer, I think some
of those issues have gone away. At least I couldn't find any problems.
And I have been running with this patch since last November and haven't
noticed anything odd.
git.c | 2 ++
| 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/git.c b/git.c
index 375e9b2..e8cff60 100644
--- a/git.c
+++ b/git.c
@@ -462,6 +462,8 @@ static void execv_dashed_external(const char **argv)
const char *tmp;
int status;
+ if (use_pager == -1)
+ use_pager = check_pager_config(argv[0]);
commit_pager_choice();
strbuf_addf(&cmd, "git-%s", argv[0]);
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index a8c6e85..742238c 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -481,4 +481,40 @@ test_expect_success TTY 'alias-specific pager can override aliased command' '
test_cmp expect actual
'
+test_expect_success 'setup external command' '
+ cat >git-external <<-\EOF &&
+ #!/bin/sh
+ git "$@"
+ EOF
+ chmod +x git-external
+'
+
+test_expect_success TTY 'command-specific pager works for external commands' '
+ sane_unset PAGER GIT_PAGER &&
+ echo "foo:initial" >expect &&
+ >actual &&
+ test_config pager.external "sed s/^/foo:/ >actual" &&
+ test_terminal git --exec-path="`pwd`" external log --format=%s -1 &&
+ test_cmp expect actual
+'
+
+test_expect_success TTY 'sub-commands of externals use their own pager' '
+ sane_unset PAGER GIT_PAGER &&
+ echo "foo:initial" >expect &&
+ >actual &&
+ test_config pager.log "sed s/^/foo:/ >actual" &&
+ test_terminal git --exec-path=. external log --format=%s -1 &&
+ test_cmp expect actual
+'
+
+test_expect_success TTY 'external command pagers override sub-commands' '
+ sane_unset PAGER GIT_PAGER &&
+ >expect &&
+ >actual &&
+ test_config pager.external false &&
+ test_config pager.log "sed s/^/log:/ >actual" &&
+ test_terminal git --exec-path=. external log --format=%s -1 &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.6.10.g62f04
^ permalink raw reply related
* [PATCH v2 0/4] Support threshold in copy/rename detection
From: Vitor Antunes @ 2011-08-18 22:20 UTC (permalink / raw)
To: git; +Cc: Pete Wyckoff, Tor Arvid Lund, Vitor Antunes
Second version that includes updates recommended by Pete Wyckoff.
Now only "true" and "false" arguments are processed, any other argument
that is no "" is passed directly.
Vitor Antunes (4):
git-p4: Allow setting rename/copy detection threshold.
git-p4: Add description of rename/copy detection options.
git-p4: Add test case for rename detection.
git-p4: Add test case for copy detection.
contrib/fast-import/git-p4 | 18 ++++--
contrib/fast-import/git-p4.txt | 25 ++++++++
t/t9800-git-p4.sh | 121 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 159 insertions(+), 5 deletions(-)
--
1.7.5.4
^ 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