* git-p4 uses -h for host instead of -H
From: Russell Myers @ 2012-02-22 0:49 UTC (permalink / raw)
To: git
In-Reply-To: <CAA5tD2sYSqtGTwW1PmFMB_mP_xG24VS6hPXTLD33bJsMaj4MWg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 425 bytes --]
In attempting to use the git-p4 plugin I ran into an issue when the host
argument was specified using the -h argument instead of the -H argument
for the host. As a result, I found that instead of git-p4 specifying a
host, it gets help information. Correcting this issue allowed me to
clone without issue. I've attached a patch of what I did to fix the
issue if this is indeed believed to be an issue.
Thanks,
Russell
[-- Attachment #2: host_flag_fix.patch --]
[-- Type: text/x-patch, Size: 884 bytes --]
>From 2f6c91282c98ca0a45269524ec74655f76921ec9 Mon Sep 17 00:00:00 2001
From: Russell Myers <mezner@russellmyers.com>
Date: Tue, 21 Feb 2012 19:18:54 -0500
Subject: [PATCH] Changing host argument to -H from -h. Based on
http://www.perforce.com/perforce/doc.current/manuals/p4guide/03_using.html
'-H' is the apporpriate flag while '-h' is a flag passed
for help content.
---
contrib/fast-import/git-p4 | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index a78d9c5..d2fd265 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -38,7 +38,7 @@ def p4_build_cmd(cmd):
host = gitConfig("git-p4.host")
if len(host) > 0:
- real_cmd += ["-h", host]
+ real_cmd += ["-H", host]
client = gitConfig("git-p4.client")
if len(client) > 0:
--
1.7.5.4
^ permalink raw reply related
* [PATCH v2] cherry-pick: No advice to commit if --no-commit
From: Phil Hord @ 2012-02-22 0:44 UTC (permalink / raw)
To: Git List, Junio C Hamano, Ramkumar Ramachandra, Jonathan Nieder,
Phil Hord
Cc: Phil Hord
When cherry-pick fails it offers a helpful hint about how to
proceed. The hint tells the user how to do the cleanup
needed by the conflicted cherry-pick and finish the job after
conflict resolution. In case of cherry-pick --no-commit, the
hint goes too far. It tells the user to finish up with
'git commit'. That is not what this git-cherry-pick was
trying to do in the first place.
Restrict the hint in case of --no-commit to avoid giving this
extra advice.
Also, add a test verifying the reduced hint for the --no-commit
version of cherry-pick.
Signed-off-by: Phil Hord <hordp@cisco.com>
---
sequencer.c | 17 +++++++++++------
t/t3507-cherry-pick-conflict.sh | 14 ++++++++++++++
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5fcbcb8..9716c63 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -123,7 +123,7 @@ static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
strbuf_release(&buf);
}
-static void print_advice(int show_hint)
+static void print_advice(int show_hint, struct replay_opts *opts)
{
char *msg = getenv("GIT_CHERRY_PICK_HELP");
@@ -138,10 +138,15 @@ static void print_advice(int show_hint)
return;
}
- if (show_hint)
- advise(_("after resolving the conflicts, mark the corrected paths\n"
- "with 'git add <paths>' or 'git rm <paths>'\n"
- "and commit the result with 'git commit'"));
+ if (show_hint) {
+ if (opts->no_commit)
+ advise(_("after resolving the conflicts, mark the corrected paths\n"
+ "with 'git add <paths>' or 'git rm <paths>'"));
+ else
+ advise(_("after resolving the conflicts, mark the corrected paths\n"
+ "with 'git add <paths>' or 'git rm <paths>'\n"
+ "and commit the result with 'git commit'"));
+ }
}
static void write_message(struct strbuf *msgbuf, const char *filename)
@@ -423,7 +428,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
msg.subject);
- print_advice(res == 1);
+ print_advice(res == 1, opts);
rerere(opts->allow_rerere_auto);
} else {
if (!opts->no_commit)
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index ee1659c..0c81b3c 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -59,6 +59,20 @@ test_expect_success 'advice from failed cherry-pick' "
test_i18ncmp expected actual
"
+test_expect_success 'advice from failed cherry-pick --no-commit' "
+ pristine_detach initial &&
+
+ picked=\$(git rev-parse --short picked) &&
+ cat <<-EOF >expected &&
+ error: could not apply \$picked... picked
+ hint: after resolving the conflicts, mark the corrected paths
+ hint: with 'git add <paths>' or 'git rm <paths>'
+ EOF
+ test_must_fail git cherry-pick --no-commit picked 2>actual &&
+
+ test_i18ncmp expected actual
+"
+
test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
pristine_detach initial &&
test_must_fail git cherry-pick picked &&
--
1.7.9.267.gda172.dirty
^ permalink raw reply related
* [PULL git-svn] various git svn updates
From: Eric Wong @ 2012-02-22 0:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Steven Walter, Steven Walter, Wei-Yin Chen,
Ævar Arnfjörð Bjarmason, Frederic Heitzmann
The following changes since commit b3a769dc355b32c95783dc07f59e4dfebdd8bdc7:
Update draft release notes to 1.7.10 (2012-02-20 00:29:40 -0800)
are available in the git repository at:
git://bogomips.org/git-svn master
for you to fetch changes up to 379862ec5a413e636d977a6ea3d618f0b3eafceb:
git-svn.perl: fix a false-positive in the "already exists" test (2012-02-21 21:37:31 +0000)
----------------------------------------------------------------
Frederic Heitzmann (1):
completion: add --interactive option to git svn dcommit
Steven Walter (2):
git-svn.perl: perform deletions before anything else
git-svn.perl: fix a false-positive in the "already exists" test
Wei-Yin Chen (陳威尹) (1):
git-svn: Fix time zone in --localtime
Ævar Arnfjörð Bjarmason (2):
git-svn: remove redundant porcelain option to rev-list
git-svn: un-break "git svn rebase" when log.abbrevCommit=true
contrib/completion/git-completion.bash | 2 +-
git-svn.perl | 68 +++++++++++++++++++-------------
t/t9100-git-svn-basic.sh | 33 +++++++++-------
3 files changed, 59 insertions(+), 44 deletions(-)
^ permalink raw reply
* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test
From: Eric Wong @ 2012-02-22 0:33 UTC (permalink / raw)
To: Steven Walter; +Cc: gitster, git
In-Reply-To: <1329747474-17976-1-git-send-email-stevenrwalter@gmail.com>
Steven Walter <stevenrwalter@gmail.com> wrote:
> open_or_add_dir checks to see if the directory already exists or not.
> If it already exists and is not a directory, then we fail. However,
> open_or_add_dir did not previously account for the possibility that the
> path did exist as a file, but is deleted in the current commit.
>
> In order to prevent this legitimate case from failing, open_or_add_dir
> needs to know what files are deleted in the current commit.
> Unfortunately that information has to be plumbed through a couple of
> layers.
>
> Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
Thanks, will push.
Acked-by: Eric Wong <normalperson@yhbt.net>
^ permalink raw reply
* Re: [PATCH] cherry-pick: No advice to commit if --no-commit
From: Phil Hord @ 2012-02-22 0:18 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano, Ramkumar Ramachandra, Phil Hord
In-Reply-To: <20120221222049.GA31934@burratino>
Jonathan Nieder <jrnieder@gmail.com> wrote:
>> - if (show_hint)
>> + if (show_hint) {
>> advise(_("after resolving the conflicts, mark the corrected paths\n"
>> - "with 'git add <paths>' or 'git rm <paths>'\n"
>> - "and commit the result with 'git commit'"));
>> + "with 'git add <paths>' or 'git rm <paths>'"));
>> + if (!opts->no_commit)
>> + advise(_( "and commit the result with 'git commit'"));
>
> "cherry-pick --no-commit" was not about to commit, but the user might
> have been. I think the hint is intended to convey that authorship
> will be correctly preserved if the user continues with "git commit"
> and no special -c option is necessary.
If that were the case, the hint would also appear when there is no conflict.
> Could you say a little more about the motivation for this patch? For
> example, did the existing message confuse someone, or was it grating
> in the context of some particular workflow?
I found it mildly confusing myself. I cherry-picked a commit with
--no-commit with no intention of committing it. I was testing how the
changes would build, but I do not need them on my branch yet. After I
resolved the conflict and tested them, I wanted to make sure there was
no lingering effects, leaving git thinking a CP was still in progress.
$ git cherry-pick --abort
error: no cherry-pick or revert in progress
Ok, so the sequencer was smart enough to leave me on my own. But just
in case, I wondered what the hint was. And I found it was not telling me
how to clean up at all, but instead telling me how to commit. It seemed
incongruous, and I assumed it was only someone's forgetting to consider
the --no-commit case.
It smelled like a bug. I started to ask about it, but it seemed easier
to just correct it and lower the list noise.
> A smaller detail: splitting the message into two like this gives
> translators less control over how to phrase the message and where to
> wrap lines. Luckily that is easy to fix with
>
> if (opts->no_commit)
> advise(...);
> else
> advise(...);
>
> which means more flexibility in phrasing the message with pertinent
> advice for each case. ;-)
I did this at first and didn't like it. I started to ask, but -- you
know, list noise.
I'll fix it in v2.
Thanks,
Phil
^ permalink raw reply
* Re: [PATCH v2] completion: remote set-* <name> and <branch>
From: SZEDER Gábor @ 2012-02-22 0:17 UTC (permalink / raw)
To: Philip Jägenstedt; +Cc: git, Felipe Contreras, Teemu Likonen
In-Reply-To: <1329861274-21418-1-git-send-email-philip@foolip.org>
Hi,
On Tue, Feb 21, 2012 at 10:54:34PM +0100, Philip Jägenstedt wrote:
> Complete <name> only for set-url. For set-branches and
> set-head, complete <name> and <branch> over the network,
> like e.g. git pull already does.
>
> The style used for incrementing and decrementing variables was fairly
> inconsistenty and was normalized to use ++x, or ((++x)) in contexts
> where the former would otherwise be interpreted as a command. This is a
> bash-ism, but for obvious reasons this script is already bash-specific.
>
> Finally, remove out-of-date documentation for how to submit patches and
> (silently) defer to Documentation/SubmittingPatches like all other code.
>
> Signed-off-by: Philip Jägenstedt <philip@foolip.org>
> ---
> contrib/completion/git-completion.bash | 42 +++++++++++++------------------
> 1 files changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1505cff..9dac084 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -60,18 +60,6 @@
> # per-repository basis by setting the bash.showUpstream config
> # variable.
> #
> -#
> -# To submit patches:
> -#
> -# *) Read Documentation/SubmittingPatches
Um, well... Did you actually read it? I mean the part that talks
about making separate commits for logically separate changes? ;)
You clearly squashed three separate changes into a single patch.
Your proposed changes to __git_complete_remote_or_refspec() and
__git_remote() make sense, modulo the increment/decrement part. I
don't have strong preference either way, but please submit those
changes in separate patches. Otherwise if ever bisect points to this
commit during a hunt for a 'git remote' completion bug, we'll need to
think about whether the increment style change or the functional
change is causing the trouble.
Thanks,
Gábor
^ permalink raw reply
* Re: [PATCH] completion: remote set-* <name> and <branch>
From: SZEDER Gábor @ 2012-02-22 0:03 UTC (permalink / raw)
To: Junio C Hamano
Cc: Philip Jägenstedt, git, SZEDER Gábor, Felipe Contreras,
Teemu Likonen
In-Reply-To: <7v8vjvg71s.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 02:23:11PM -0800, Junio C Hamano wrote:
> >>> {
> >>> local cur_="$cur" cmd="${words[1]}"
> >>> local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
> >>> + if [ "$cmd" = "remote" ]; then
> >>> + c=$((++c))
> >>> + fi
> >>
> >> I don't know about others, but auto-incrementing a variable and assigning
> >> the result to the same variable, while not wrong at all, hurts my brain.
> >>
> >> c=$(($c + 1))
> >>
> >> is far more readable and does not suggest there is any funky magic
> >> involved. Also it is a good habit to get into not to omit $ from
> >> variables inside arithmetic substitution, even though bash allows it and
> >> this script is meant to be consumed only by shells that understand this
> >> bash-ism.
> >>
> >> I do not know offhand if zsh groks it, but the point is that you do not
> >> have to worry about it if you write "$(($c+1))" instead of "$((c+1))".
This c=$((++c)) style increment is used in several completion
functions, even in the main entry function _git() itself. If zsh
didn't grok it, then completion wouldn't work at all on zsh, because
the while loop in _git() would be an endless loop.
> > CodingGuidlines suggests to follow local convention, which was
> > "c=$((++c))". This file also uses "++n", "i++" and "((i++))". I will
> > send a v2 patch that normalizes these, open to discussion of course.
>
> OK, it was my mistake to suggest $c++ in this file; it liberally uses
> dollar-less variables, and I agree that it is a good idea to stick to that
> local convention.
>
> But I think you went too far in your follow-up patch to make the increment
> and decrement uniform.
>
> "i++" is so much easier on the eye unless you must use "++i" in order to
> use the value of the incremented "i" in an expression, and the changes to
> turn existing instances of free-standing "i++" to "++i" done only for the
> side effect of incrementing the variables look totally backwards.
>
> Although I do not deeply care. Just leaving the new one as you originally
> wrote, i.e.
>
> c=$((++c))
>
> would have been easier to review for the area experts, I would think.
The issue of this weird increment came up before
http://thread.gmane.org/gmane.comp.version-control.git/87650/focus=87806
and the conclusion back then was that we kept the existing style. But
otherwise I agree, it hurts my brain, too.
Best,
Gábor
^ permalink raw reply
* Re: [RFC] pre-rebase: Refuse to rewrite commits that are reachable from upstream
From: Dave Zarzycki @ 2012-02-21 23:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johan Herland, git, jnareb, philipoakley
In-Reply-To: <7vsji3eoqs.fsf@alter.siamese.dyndns.org>
On Feb 21, 2012, at 6:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> history"[1], but won't cause any false positives (and small enough
>> false negatives). If there really is no way to implement this, then we
>> shouldn't give users false hopes by putting it in the survey...
>
> I think that question should be "warn before pushing out a commit that the
> user may later regret to have pushed out" ;-)
Why limit this proposal to just the commits that are reachable from upstream? What if somebody pulls from your repo?
In other words, wouldn't it be better to have a git track "unshared" commits and only let those be rewritten? The theory being that if the given commits haven't been pushed or pulled anywhere, then they are safe to rewrite.
davez
^ permalink raw reply
* Re: how do you review auto-resolved files
From: Junio C Hamano @ 2012-02-21 23:52 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: git
In-Reply-To: <ji0vik$e48$1@dough.gmane.org>
"Neal Kreitzinger" <neal@rsss.com> writes:
> If I expect/assume you are going to review the
> auto-resolutions after-the-fact then you can neglect this because you:
>
> - have become complacent that git usually does-what-you-want so "you don't
> really need to do it",
> - are lazy and do it half-way,
> - forget to do it,
> - think "git magically does your work for you",
> - don't know how to do it,
> - don't even realize that anything auto-resolved or what auto-resolved,
> - decide you don't have to do it because that is what testing if for,
> - you think that your time is so valuable that an ounce-of-prevention on
> your part is not worth a pound-of-cure on the part of others.
A couple more bullet points I can think of off the top of my head, after
making sure that you do not count what "rerere" does as part of the
"auto-resolution", to add to the above list are:
- know git is stupid and errs on the safe side, punting anything remotely
complex;
- know that textual non-conflicts that occur in the same file have the
same risk of having semantic conflict across different files, so
singling out "touched the same file but did not conflict" any special
is pointless, but in either case, the chance of having such a conflict
is small enough that completing the merge (and other merges) first and
then checking the overall result is more efficient use of your time,
because you have to eyeball the result at least once anyway before
pushing it out.
^ permalink raw reply
* Re: [RFC] pre-rebase: Refuse to rewrite commits that are reachable from upstream
From: Junio C Hamano @ 2012-02-21 23:43 UTC (permalink / raw)
To: Johan Herland; +Cc: Junio C Hamano, git, jnareb, philipoakley
In-Reply-To: <CALKQrgfLBKG5ssL8ua_EtfyQeOp0h9HPC7vdOM_Yc_AVaa39xw@mail.gmail.com>
Johan Herland <johan@herland.net> writes:
> history"[1], but won't cause any false positives (and small enough
> false negatives). If there really is no way to implement this, then we
> shouldn't give users false hopes by putting it in the survey...
I think that question should be "warn before pushing out a commit that the
user may later regret to have pushed out" ;-)
^ permalink raw reply
* Re: [RFC] pre-rebase: Refuse to rewrite commits that are reachable from upstream
From: Johan Herland @ 2012-02-21 23:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jnareb, philipoakley
In-Reply-To: <7vehtoiqae.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 08:44, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> What you are describing here may be a common workflow, but
>> "rebase.bottomLimit" is still very specific to that kind of workflow.
>> What I'm after is a much more workflow-agnostic concept of:
>>
>> "If I have pushed something, I should probably not rebase it"
>
> Your "this branch pushes directly to that remote branch, so I can check if
> it will result in rewrite of published commit" is even *less* generic than
> having a single bottomLimit in my illustration.
>
> I may not push out my topic branches directly, only the aggregate of them
> in 'next', but once 'next' is pushed out, they are not eligible for
> rebasing. A per-branch bottom, e.g. rebase.$branch.bottomLimit, might
> make it more flexible to cover such a case, though.
But you would have to maintain rebase.$branch.bottomLimit separately
for each topic branch, updating it whenever the topic branch is merged
to 'next'. Is that practical?
> On the other hand, without any such safety, a merge to 'next' would give
> many conflicts and "shortlog master..next" will show many duplicates after
> any topic that are already merged to it are accidentally rewritten, and it
> is just the matter of using reflog on topic branches to recover from such
> a mistake.
All true, but my experience is that novice users will not enjoy this
experience, and rather become frustrated with Git for "putting them in
this situation" (i.e. giving them more rope than they can handle).
Maybe I'm doing something wrong...
>>> I wonder if it would be a more direct solution to the issue you are
>>> raising to give them a good tool to help them to be more careful with less
>>> effort on their part before they publish (not before they rebase).
>>
>> ..., I'm not sure how we can help the user _not_ publish the
>> branch until it's ready.
>
> I think we are in agreement that we do not think of a good solution
> offhand to the real cause of the issue, except by encouraging the use of
> throw-away review branches, perhaps.
Agreed. We should probably focus more on this when teaching novice users.
>> I think the following decribes what often happens for many users:
>>
>> 1. User A pushes the branch to a public repo.
>> 2. User B points out a simple mistake in the branch.
>
> That's the CVS workflow, and it is not "a" public repo but "the" public
> repo shared between A and B (and also with all the project participants).
>
>> 3. User A makes a fix
>> 4. User A squashes the fix into the (already-published) history.
>> 5. User A attempts to push the "fixed" history (but is rejected by
>> the public repo because of non-fast-forward).
>> At this point, the damage is already done,...
>
> Which is probably a sufficient safety which the user can learn from. If
> this happens too often, that probably means we are not helping them enough
> to learn not to "commit --amend" or "rebase" if they are using Git as a
> better CVS.
>
>> You could say that User A should be more careful and push to a "less
>> public" repo in step #1 (thus allowing the fix to be squashed before
>> pushing to a "more public" repo in step #5),...
>
> That is essentially a workflow that uses throw-away review branches in a
> distributed environment, and at that point, we are not constrained by the
> limitation of the CVS workflow. While still in early review cycles (which
> corresponds to being in our 'pu'), "commit --amend" and "rebase" are fine
> tool to be used. And...
>
>> but how "public" is
>> "public" enough to have someone point out the bug, but still
>> "unpublic" enough to allow rebase?
>
> ... I can imagine that currently that is determined purely by project
> convention. Perhaps there needs a way to mark throw-away review branches
> like 'pu' (or saying the same thing from the different perspective, to
> mark cast-in-stone integration branches like 'next') so that tools can
> mechanically decide what should and should not be rewritten.
Yes. Maybe it should be a tri-state thing:
- "rebase-prone" will allow rewrites without asking.
- unset will warn, but still allow rewrites. The warning should
explain the potential problem that may arise from rewriting, and
should also explain how to get rid of the warning by setting this
config to either of the above/below options. Ideally the warning
should also explain how the user may undo the rewrite. Obviously, most
of this warning should be controlled by a corresponding advice.*
config.
- "cast-in-stone" will refuse rewrites.
I notice that this config is not really per-branch, but rather per
upstream branch. I wonder how to best encode that in the config...
Also, how a remote repo may best communicate which branches are
"rebase-prone"/"cast-in-stone".
> To extend the idea of promoting throw-away review branches further,
> perhaps it might help if there is an easy way to let the users publish
> their "master" to a branch that is not the "master" of the central shared
> repository even in the CVS workflow (e.g. by default a push from user A
> always goes to refs/review/A/master), and to have an option to "git push"
> that makes it go to the "master" when the user really means the branch is
> ready (and it would move refs/review/A/master to attic to be later gc'ed).
This goes against many workflows, and many users' expectations.
Although it may be a Good(tm) practice, I don't think it's universal
enough to be worth "breaking" 'push'. What about adding a new 'git
review' command which defaults to this behavior (but can be overridden
by Gerrit and other code-review systems to do what's appropriate in
their cases)?
>> ... And I
>> think that refusing rewrites of commits that are already present in
>> the @{upstream} remote-tracking branch is good enough to help most
>> users avoid steps #4 through #6 (in a push-based workflow[1]).
>
> See above regarding branches that should not be rebased even if they are
> not directly pushed out.
True. Is it universally acceptable to assume that a topic branch that
has been merged to a "cast-in-stone" branch, should not be
rebased/rewritten (unless forced by the user)?
There are several aspects to this: First, any commits on the topic
branch that have been made _since_ the merge should obviously be
rewritable (since they have yet to be merged). And commits made
_before_ the merge should probably not be rewritable. I.e. in the
following figure, the 'y' commits should be rewritable, but the 'x'
commits (preceding the merge 'M') should probably not.
--o---o---o---o---o---o---M <- master (cast-in-stone)
\ /
x---x---x---x---x---x---y---y <- topicA
But what if we have two topic branches with shared history, like this:
--o---o---o---o---o---o---M <- master (cast-in-stone)
\ /
x---x---x---x---x---x---y---y <- topicA
\
z---z <- topicB
We cannot say whether the 'x' commits "belong" to topicA or topicB.
Also, we cannot determine whether the merge M happened with topicA or
topicB (unless the default merge message has been preserved). Now,
given that we try to rewrite the entire topicB branch (including the
'x' commits): Should we refuse rewriting the 'x' commits because they
are reachable from master (regardless of whether topicA or topicB was
merged), or should we allow rewriting the 'x' commits on the basis
that topicA may be the branch that was merged in 'M', and since topicB
are "unaware" that the 'x' commits have been merged, it should thus be
allowed to rewrite them?
Now, what if you want to backport 'topicA' from 'master' to 'maint'.
You could do so like this:
git checkout -b topicA_maint topicA
git rebase --onto maint master
(note that topicA_maint is a special case of topicB in the above
graph) In this case, we would expect the 'x' commits to be rewritable,
or we could not perform the backport.
The conclusion seems to be that we _cannot_ refuse rewriting commits
merely on the basis that they have been merged to a cast-in-stone
branch.
>> In
>> fact, from a pedagogical POV, I think step #4 is probably the best
>> spot for novice users to learn exactly the distinction between
>> acceptable and unacceptable history rewrites (instead of having it
>> explained to them as a consequence of the step #5).
>
> I doubt you have enough information at point #4, unless you restrict the
> workflow you allow your novice users to use fairly severely, to give
> appropriate advice. While I agree with you that it would be the best if
> we could do so at step #4 without stopping the user from doing what s/he
> needs to do with false positive, I think it is not pedagogical POV but
> dreaming if the world were ideal without knowing what it would take to
> make it ideal.
>
> At least I don't know offhand what kind of changes are needed to restrict
> the user actions to an "approved" workflow so that step #4 can make a
> useful decision (that is, no false positives and small enough false
> negatives).
Ok, trying to make as few assumptions about user workflows as
possible: Assuming there were a (machine-parsable) way to mark
branches as either "rebase-prone" or "cast-in-stone". Could we then
assume that rebasing commits that exist on a cast-in-stone branch's
remote-tracking @{upstream} should be refused by default? I'm trying
(without much success, it seems) to find _something_ that will help
the 1525 users that want Git to "warn before/when rewriting published
history"[1], but won't cause any false positives (and small enough
false negatives). If there really is no way to implement this, then we
shouldn't give users false hopes by putting it in the survey...
Have fun! :)
...Johan
[1]: Question 17 in https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: how do you review auto-resolved files
From: Neal Kreitzinger @ 2012-02-21 23:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Neal Kreitzinger, git
In-Reply-To: <7vhayjga0a.fsf@alter.siamese.dyndns.org>
On 2/21/2012 3:19 PM, Junio C Hamano wrote:
> "Neal Kreitzinger"<neal@rsss.com> writes:
>
>> When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file
>> changes that do not conflict on the same line(s).
>>
>> Technical Question: What are the recommended commands for reviewing the
>> files that auto-resolved after a "merge"?
>
> Imagine that you are the maintainer of the mainline and are reviewing the
> work made on a side branch that you just merged, but pretend that the
> contribution came as a patch instead. How would you assess the damage to
> your mainline?
>
> You would use "git show --first-parent $commit" for that.
>
> And then look at what the sideline wanted to do to the old baseline:
>
> git log -p $commit^..$commit
>
> which would, unless the person who worked on the side branch did a shoddy
> job describing his work, explain what the side branch wanted to achieve
> and also _how_ it wanted to achieve it.
>
> And then re-read the first "git show" output with that knowledge, together
> with the knowledge you have on your mainline codebase, and decide if the
> solution used by the side branch is still valid. If it makes sense, you
> are done. If the advance in your mainline since the side branch forked
> invalidated some assumption the side branch made (e.g. a helper function
> the side branch used has changed its meaning, a helper function the side
> branch changed its meaning gained more callsite on the mainline, etc.),
> you have a semantic conflict that you would need to address.
>
> It is unclear what exactly you consider "auto-resolve" in your message, so
> I'd refrain from commenting on the "Philosophical" part, at least for now.
Context: (git-merge manpage definition of merge-conflict) "During a
merge, the working tree files are updated to reflect the result of the
merge... When both sides made changes to the same area, however, git
cannot randomly pick one side over the other, and asks you to resolve it
by leaving what both sides did to that area."
My definition for "auto-resolve": "During a merge, the working tree
files are updated to reflect the result of the merge... When both sides
made changes to different areas of the same file, git picks both sides
automatically, and leaves its up to you to make sure you review those
merge results for correctness after git has made the merge commit."
IOW, an "auto-resolve" specifically means that both sides (ours and
theirs) made changes to file(a) since the common-ancestor version of
fila(a), and git picked both sides without raising a merge-conflict.
(The reason I came up with the term "auto-resolve" is because in the
git-merge output the term "Auto-merging" can also indicate that only one
side (theirs) changed file(a) since the common-ancestor and that git is
just "fast-forwarding" theirs file(a) on top of common-ancestor file(a).)
v/r,
neal
^ permalink raw reply
* Re: Working with remotes with (too) many branches
From: Junio C Hamano @ 2012-02-21 22:47 UTC (permalink / raw)
To: Philip Jägenstedt; +Cc: Junio C Hamano, git, Jonathan Nieder
In-Reply-To: <CAKHWUkY_bEtKoPOiWFz6i2bp-tbekapROOaCYR0k5Ymh0HYMdA@mail.gmail.com>
Philip Jägenstedt <philip@foolip.org> writes:
> This looks a bit odd to me, shouldn't the new setting file under
> [remote "origin"]?
Yeah, remote.$name.fetchDefault or something.
> Thanks for the tip. (I did look at the original commit and search the
> archives, but found no answers.)
Heh, it does say --delete # not implemented ;-)
^ permalink raw reply
* Re: Working with remotes with (too) many branches
From: Jonathan Nieder @ 2012-02-21 22:43 UTC (permalink / raw)
To: Philip Jägenstedt; +Cc: Junio C Hamano, git, martin f. krafft
In-Reply-To: <CAKHWUkY_bEtKoPOiWFz6i2bp-tbekapROOaCYR0k5Ymh0HYMdA@mail.gmail.com>
Hi Philip,
Philip Jägenstedt wrote:
> It seems to me that if one starts out with all branches and then
> "set-branches main footopic", one really does want all other refs to
> go away on the next fetch.
I would expect set-branches to remove the remote-tracking branches
that are no longer relevant itself, actually.
Martin, does this seem sensible to you? If so, I'll be happy to look
into it in the next few days (or I'll be even happier someone else
gets to it first).
Hope that helps,
Jonathan
^ permalink raw reply
* Re: Working with remotes with (too) many branches
From: Philip Jägenstedt @ 2012-02-21 22:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jonathan Nieder
In-Reply-To: <7v7gzipkt8.fsf@alter.siamese.dyndns.org>
On Sun, Feb 19, 2012 at 22:36, Junio C Hamano <gitster@pobox.com> wrote:
> Philip Jägenstedt <philip@foolip.org> writes:
>
>> 1. If I make a typo with remote set-branches, fetch will fail with
>> "fatal: Couldn't find remote ref refs/heads/typotopic" and not fetch
>> anything at all.
>
> At that point you can notice the earlier typo and remove or fix the fetch
> refspec you have.
>
> Alternatively, set-branches could run ls-remote against the remote and
> notice that there is no such branch over there. However, even if you got
> the branch name right when you did "set-branches", you would still see the
> same "Couldn't find" when the branch gets removed over there, so you would
> need a way to remove or fix the fetch refspec you have *anyway*.
>
> So, assuming that there is no easy way to remove one branch from the set
> of branches tracked from a given remote, it is much more important to add
> such a way. Checking against a typo when "set-branches" is run is "nicer
> to have" but lack of it is not a show-stopper.
>
> Wouldn't "git config --unset remote.origin.fetch '/typotopic'" be
> sufficient in the meantime even if a user fears "vi .git/config"?
Yeah, one can recover if one knows what config "set-branches" maps to,
but I'd like this to be less error-prone so that I can recommend it to
others suffering from branch explosion. Tab completion and
"set-branches --remove" is probably a good start.
>> 2. If I forget that I've previously worked with footopic and
>> set-branches --add it again, I'll get a duplicate line in my config.
>
> I do not know the duplicate hurts anything, but I agree that it would be
> more aescetically pleasing if "--add" noticed what you already had and
> avoided duplicates.
Right, it's not worthwhile by itself, but something to consider if
implementing --delete.
>> 3. When I don't care about footopic anymore, there's no clear way to
>> stop fetching it and remove refs/remotes/main/footopic.
>
> Isn't the lack of "set-branches --delete" the same as #1 above?
Kind of, several of these points can be solved by the same fixes.
> The
> latter would be "branch -r -d main/footopic" but I could imagine
> "set-branches --delete" would do that for you once implemented.
Letting "set-branches --delete" manage this per-branch would be nice, I agree.
Would it be wrong, though, if "fetch --prune" and "remote prune"
simply pruned all refs under refs/remotes/main/ that were not fetched?
It seems to me that if one starts out with all branches and then
"set-branches main footopic", one really does want all other refs to
go away on the next fetch.
>> 4. If set-branches --delete existed one could end up with no fetch
>> lines in the remote config, at which point fetch falls back to
>> fetching HEAD, instead of the expected nothing.
>
> Don't do that, then ;-)
>
> I could imagine a new preference "fetch.$remote.default=nothing" that
> causes "git fetch" to fail with "You have fetch.$remote.default=nothing
> set, so I am not fetching anything from there without any configured
> refspec". The default would be fetch.$remote.default=HEAD, I would guess.
>
> The preference can be set automatically when you use "set-branches"
> without "--add" for a given remote, as use of "set-branches" is a clear
> indication that you want to deviate from the built-in default behaviour
> when interacting with that remote.
That sounds like a good idea. To make sure that I understand the
notation, the actual config would look something like this:
[remote "origin"]
url = git://github.com/gitster/git.git
# no fetch, it was removed
[fetch "origin"]
default = nothing
This looks a bit odd to me, shouldn't the new setting file under
[remote "origin"]?
>> I'd appreciate feedback on these issues so that I don't waste time
>> trying to patch the wrong problems. Suggestions for an alternative
>> work flow is of course also most welcome!
>
> Admittedly I wouldn't use "set-branches" myself (it is far easier to tweak
> with "vi .git/config", and I wasn't involved in), but looking at the whole
> history of the feature with "git log --grep=set-branch builtin/remote.c",
> I have a feeling that not many people used, cut by its still-unrounded
> edges, and polished the subcommand by rounding them out yet, and it has a
> large room for improvement.
That was my impression as well.
> It would have saved you time to wait for a round-trip if you did the above
> "git log" yourself to find out from whom this subcommand came from, and
> looked at list archive to see if the original author is still active here
> (in this case he is), and Cc'ed him before posting the message I am
> responding to.
Thanks for the tip. (I did look at the original commit and search the
archives, but found no answers.)
--
Philip Jägenstedt
^ permalink raw reply
* Re: [PATCH] cherry-pick: No advice to commit if --no-commit
From: Junio C Hamano @ 2012-02-21 22:31 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Phil Hord, Git List, Ramkumar Ramachandra, Phil Hord
In-Reply-To: <20120221222049.GA31934@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
>> + if (show_hint) {
>> advise(_("after resolving the conflicts, mark the corrected paths\n"
>> + "with 'git add <paths>' or 'git rm <paths>'"));
>> + if (!opts->no_commit)
>> + advise(_( "and commit the result with 'git commit'"));
>
> "cherry-pick --no-commit" was not about to commit, but the user might
> have been. I think the hint is intended to convey that authorship
> will be correctly preserved if the user continues with "git commit"
> and no special -c option is necessary.
Actually, an often used but perhaps neglected use case of "cherry-pick --no-commit"
is to use it as a substitute for
git show $that_commit | git apply [--index]
to lift the change from $that_commit and incorporate it into your
unrelated (from the point of view of the author of $that_commit) work.
The user would be doing "edit $this_file" and "edit $that_file" before or
after the "show | apply" aka "cherry-pick --no-commit", and it is merely
a type-saver, not having to re-type the change $that_commit introduced
relative to its parent.
So in that context, I can understand the suggestion to commit is at best
annoying ("I am still working on the current change. Why do you talk about
committing?") and at worst alarming and misleading ("Oh, am I to lose some
change if I do not commit right now?").
> A smaller detail: splitting the message into two like this gives
> translators less control over how to phrase the message and where to
> wrap lines. Luckily that is easy to fix with
>
> if (opts->no_commit)
> advise(...);
> else
> advise(...);
Yes, this pattern must be followed in the reroll.
Thanks for a review and comment.
^ permalink raw reply
* Re: [PATCH] cherry-pick: No advice to commit if --no-commit
From: Jonathan Nieder @ 2012-02-21 22:23 UTC (permalink / raw)
To: Phil Hord; +Cc: Git List, Junio C Hamano, Ramkumar Ramachandra, Phil Hord
In-Reply-To: <1329858317-3066-1-git-send-email-hordp@cisco.com>
Hi Phil,
Phil Hord wrote:
> Signed-off-by: Phil Hord <hordp@cisco.com>
Thanks.
> +++ b/sequencer.c
[...]
> @@ -138,10 +138,12 @@ static void print_advice(int show_hint)
> return;
> }
>
> - if (show_hint)
> + if (show_hint) {
> advise(_("after resolving the conflicts, mark the corrected paths\n"
> - "with 'git add <paths>' or 'git rm <paths>'\n"
> - "and commit the result with 'git commit'"));
> + "with 'git add <paths>' or 'git rm <paths>'"));
> + if (!opts->no_commit)
> + advise(_( "and commit the result with 'git commit'"));
"cherry-pick --no-commit" was not about to commit, but the user might
have been. I think the hint is intended to convey that authorship
will be correctly preserved if the user continues with "git commit"
and no special -c option is necessary.
Could you say a little more about the motivation for this patch? For
example, did the existing message confuse someone, or was it grating
in the context of some particular workflow?
A smaller detail: splitting the message into two like this gives
translators less control over how to phrase the message and where to
wrap lines. Luckily that is easy to fix with
if (opts->no_commit)
advise(...);
else
advise(...);
which means more flexibility in phrasing the message with pertinent
advice for each case. ;-)
Ciao,
Jonathan
^ permalink raw reply
* Re: [PATCH] completion: remote set-* <name> and <branch>
From: Junio C Hamano @ 2012-02-21 22:23 UTC (permalink / raw)
To: Philip Jägenstedt
Cc: git, SZEDER Gábor, Felipe Contreras, Teemu Likonen
In-Reply-To: <CAKHWUkZjQYnf=LwS_RC-E_7gV73AzbWjxs33E5-FL25s6_qX4g@mail.gmail.com>
Philip Jägenstedt <philip@foolip.org> writes:
> Thanks. Perhaps git-completion.bash should not say "Send all patches
> to the current maintainer" and simply defer to SubmittingPatches?
I see you did this in your follow-up patch. Thanks.
>>> {
>>> local cur_="$cur" cmd="${words[1]}"
>>> local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>>> + if [ "$cmd" = "remote" ]; then
>>> + c=$((++c))
>>> + fi
>>
>> I don't know about others, but auto-incrementing a variable and assigning
>> the result to the same variable, while not wrong at all, hurts my brain.
>>
>> c=$(($c + 1))
>>
>> is far more readable and does not suggest there is any funky magic
>> involved. Also it is a good habit to get into not to omit $ from
>> variables inside arithmetic substitution, even though bash allows it and
>> this script is meant to be consumed only by shells that understand this
>> bash-ism.
>>
>> I do not know offhand if zsh groks it, but the point is that you do not
>> have to worry about it if you write "$(($c+1))" instead of "$((c+1))".
>
> CodingGuidlines suggests to follow local convention, which was
> "c=$((++c))". This file also uses "++n", "i++" and "((i++))". I will
> send a v2 patch that normalizes these, open to discussion of course.
OK, it was my mistake to suggest $c++ in this file; it liberally uses
dollar-less variables, and I agree that it is a good idea to stick to that
local convention.
But I think you went too far in your follow-up patch to make the increment
and decrement uniform.
"i++" is so much easier on the eye unless you must use "++i" in order to
use the value of the incremented "i" in an expression, and the changes to
turn existing instances of free-standing "i++" to "++i" done only for the
side effect of incrementing the variables look totally backwards.
Although I do not deeply care. Just leaving the new one as you originally
wrote, i.e.
c=$((++c))
would have been easier to review for the area experts, I would think.
^ permalink raw reply
* [PATCH v2] completion: remote set-* <name> and <branch>
From: Philip Jägenstedt @ 2012-02-21 21:54 UTC (permalink / raw)
To: git
Cc: SZEDER Gábor, Felipe Contreras, Teemu Likonen,
Philip Jägenstedt
In-Reply-To: <CAKHWUkZjQYnf=LwS_RC-E_7gV73AzbWjxs33E5-FL25s6_qX4g@mail.gmail.com>
Complete <name> only for set-url. For set-branches and
set-head, complete <name> and <branch> over the network,
like e.g. git pull already does.
The style used for incrementing and decrementing variables was fairly
inconsistenty and was normalized to use ++x, or ((++x)) in contexts
where the former would otherwise be interpreted as a command. This is a
bash-ism, but for obvious reasons this script is already bash-specific.
Finally, remove out-of-date documentation for how to submit patches and
(silently) defer to Documentation/SubmittingPatches like all other code.
Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
contrib/completion/git-completion.bash | 42 +++++++++++++------------------
1 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1505cff..9dac084 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -60,18 +60,6 @@
# per-repository basis by setting the bash.showUpstream config
# variable.
#
-#
-# To submit patches:
-#
-# *) Read Documentation/SubmittingPatches
-# *) Send all patches to the current maintainer:
-#
-# "Shawn O. Pearce" <spearce@spearce.org>
-#
-# *) Always CC the Git mailing list:
-#
-# git@vger.kernel.org
-#
if [[ -n ${ZSH_VERSION-} ]]; then
autoload -U +X bashcompinit && bashcompinit
@@ -395,7 +383,7 @@ __git_reassemble_comp_words_by_ref()
fi
# List of word completion separators has shrunk;
# re-assemble words to complete.
- for ((i=0, j=0; i < ${#COMP_WORDS[@]}; i++, j++)); do
+ for ((i=0, j=0; i < ${#COMP_WORDS[@]}; ++i, ++j)); do
# Append each nonempty word consisting of just
# word separator characters to the current word.
first=t
@@ -408,7 +396,7 @@ __git_reassemble_comp_words_by_ref()
# Attach to the previous token,
# unless the previous token is the command name.
if [ $j -ge 2 ] && [ -n "$first" ]; then
- ((j--))
+ ((--j))
fi
first=
words_[$j]=${words_[j]}${COMP_WORDS[i]}
@@ -416,7 +404,7 @@ __git_reassemble_comp_words_by_ref()
cword_=$j
fi
if (($i < ${#COMP_WORDS[@]} - 1)); then
- ((i++))
+ ((++i))
else
# Done.
return
@@ -738,6 +726,9 @@ __git_complete_remote_or_refspec ()
{
local cur_="$cur" cmd="${words[1]}"
local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+ if [ "$cmd" = "remote" ]; then
+ ((++c))
+ fi
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
@@ -755,7 +746,7 @@ __git_complete_remote_or_refspec ()
-*) ;;
*) remote="$i"; break ;;
esac
- c=$((++c))
+ ((++c))
done
if [ -z "$remote" ]; then
__gitcomp_nl "$(__git_remotes)"
@@ -788,7 +779,7 @@ __git_complete_remote_or_refspec ()
__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
fi
;;
- pull)
+ pull|remote)
if [ $lhs = 1 ]; then
__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
else
@@ -995,7 +986,7 @@ __git_find_on_cmdline ()
return
fi
done
- c=$((++c))
+ ((++c))
done
}
@@ -1006,7 +997,7 @@ __git_has_doubledash ()
if [ "--" = "${words[c]}" ]; then
return 0
fi
- c=$((++c))
+ ((++c))
done
return 1
}
@@ -1129,7 +1120,7 @@ _git_branch ()
-d|-m) only_local_ref="y" ;;
-r) has_r="y" ;;
esac
- c=$((++c))
+ ((++c))
done
case "$cur" in
@@ -2289,7 +2280,7 @@ _git_config ()
_git_remote ()
{
- local subcommands="add rename rm show prune update set-head"
+ local subcommands="add rename rm set-head set-branches set-url show prune update"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
@@ -2297,9 +2288,12 @@ _git_remote ()
fi
case "$subcommand" in
- rename|rm|show|prune)
+ rename|rm|set-url|show|prune)
__gitcomp_nl "$(__git_remotes)"
;;
+ set-head|set-branches)
+ __git_complete_remote_or_refspec
+ ;;
update)
local i c='' IFS=$'\n'
for i in $(git --git-dir="$(__gitdir)" config --get-regexp "remotes\..*" 2>/dev/null); do
@@ -2580,7 +2574,7 @@ _git_tag ()
f=1
;;
esac
- c=$((++c))
+ ((++c))
done
case "$prev" in
@@ -2633,7 +2627,7 @@ _git ()
--help) command="help"; break ;;
*) command="$i"; break ;;
esac
- c=$((++c))
+ ((++c))
done
if [ -z "$command" ]; then
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH] contrib: added git-diffall
From: Junio C Hamano @ 2012-02-21 21:51 UTC (permalink / raw)
To: Tim Henigan; +Cc: git
In-Reply-To: <1329785969-828-1-git-send-email-tim.henigan@gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> diff --git a/contrib/diffall/git-diffall b/contrib/diffall/git-diffall
> new file mode 100755
> index 0000000..ef01eda
> --- /dev/null
> +++ b/contrib/diffall/git-diffall
> @@ -0,0 +1,275 @@
> +#!/bin/bash -e
Does this have to be bash-only (iow, infested with bash-isms beyond
repair), or is you wrote this merely from inertia?
The following is only after a cursory scanning, so there may be other
things that needs fixing, but anyway:
- Don't use "which" in scripts. Its output is not machine parseable, and
exit code is not reliable, across platforms. It is only meant for
consumption by human who can read English (or natural language in the
current locale).
> + if test -z "$paths"
> + then
> + paths=$1
> + else
> + paths="$paths $1"
> + fi
Just a style tip; if you are going to let shell $IFS split this list
anyway, it is customary to write the above as
paths="$paths$1 "
> + git diff --name-only "$left"..."$right" -- $paths > "$tmp"/filelist
git diff ... -- $paths >"$tmp/filelist"
> +# Exit immediately if there are no diffs
> +if test ! -s "$tmp"/filelist
> +then
> + exit 0
> +fi
> +
> +if test -n "$copy_back" && test "$right_dir" != "working_tree"
> +then
> + echo "--copy-back is only valid when diff includes the working tree."
> + exit 1
> +fi
> +
> +# Create the named tmp directories that will hold the files to be compared
> +mkdir -p "$tmp"/"$left_dir" "$tmp"/"$right_dir"
mkdir -p "$tmp/$left_dir" "$tmp/$right_dir"
> + if test -n "$compare_staged"
> + then
> + ls_list=$(git ls-tree HEAD $name)
> + if test -n "$ls_list"
> + then
> + mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
> + git show HEAD:"$name" > "$tmp"/"$left_dir"/"$name"
> + fi
> + else
> + mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
> + git show :"$name" > "$tmp"/"$left_dir"/"$name"
> + fi
That's misleadingly indented. First I thought "in what case would we want
to switch the LHS between HEAD:$path and :$path when doing diff --cached?"
but the overindented four lines starting from the funny "fi" is about non
cached case.
> + fi
> +done < "$tmp"/filelist
> +
> +cd "$tmp"
> +LOCAL="$left_dir"
> +REMOTE="$right_dir"
> +
> +if test -n "$diff_tool"
> +then
> + export BASE
> + eval $diff_tool '"$LOCAL"' '"$REMOTE"'
> +else
> + run_merge_tool "$merge_tool" false
> +fi
> +
> +# This function is called on exit
> +cleanup () {
> + cd "$start_dir"
> +
> + # Copy files back to the working dir, if requested
> + if test -n "$copy_back" && test "$right_dir" = "working_tree"
> + then
> + git_top_dir=$(git rev-parse --show-toplevel)
> + find "$tmp/$right_dir" -type f|while read file; do
> + cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
> + done
Why is this loop written in such a dense way? Everything else (except for
that misindented part) were almost to our CodingStyle and was fairly easy
to read, though.
> + fi
> +
> + # Remove the tmp directory
> + rm -rf "$tmp"
> +}
> +
> +trap cleanup EXIT
Does this even trigger? This is not Perl that parses and runs set-up code
before executing everything else, so I suspect this last line amounts to
the same thing as writing just
cleanup
without trap nor signal names.
If you are to set up temporary files or directories that you want to clean
up, a good discipline is to follow this order:
- define variable(s) to hold the temporary locations, e.g.
tmpdir=$(mktemp ...)
- set the trap before starting to use these temporary locations, e.g.
trap 'rm -rf "$tmpdir' 0 1 2 3 15
- and then start populating tmpdir and do whatever you want to do.
^ permalink raw reply
* Re: [PATCH] completion: remote set-* <name> and <branch>
From: Philip Jägenstedt @ 2012-02-21 21:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, SZEDER Gábor, Felipe Contreras, Teemu Likonen
In-Reply-To: <7vvcn2lyw6.fsf@alter.siamese.dyndns.org>
On Mon, Feb 20, 2012 at 08:58, Junio C Hamano <gitster@pobox.com> wrote:
> Philip Jägenstedt <philip@foolip.org> writes:
>
>> Complete <name> only for set-url. For set-branches and
>> set-head, complete <name> and <branch> over the network,
>> like e.g. git pull already does.
>>
>> Signed-off-by: Philip Jägenstedt <philip@foolip.org>
>
> You addressed your patch to Shawn, who originally wrote this, but
>
> "git shortlog -n -s --no-merges --since=9.months pu contrib/completion"
>
> indicates that he no longer is involved in enhancing this script, and it
> has seen actions primarily from three people I Cc'ed this message to.
Thanks. Perhaps git-completion.bash should not say "Send all patches
to the current maintainer" and simply defer to SubmittingPatches?
>> contrib/completion/git-completion.bash | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 1505cff..8e7abb6 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -738,6 +738,9 @@ __git_complete_remote_or_refspec ()
>> {
>> local cur_="$cur" cmd="${words[1]}"
>> local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
>> + if [ "$cmd" = "remote" ]; then
>> + c=$((++c))
>> + fi
>
> I don't know about others, but auto-incrementing a variable and assigning
> the result to the same variable, while not wrong at all, hurts my brain.
>
> c=$(($c + 1))
>
> is far more readable and does not suggest there is any funky magic
> involved. Also it is a good habit to get into not to omit $ from
> variables inside arithmetic substitution, even though bash allows it and
> this script is meant to be consumed only by shells that understand this
> bash-ism.
>
> I do not know offhand if zsh groks it, but the point is that you do not
> have to worry about it if you write "$(($c+1))" instead of "$((c+1))".
CodingGuidlines suggests to follow local convention, which was
"c=$((++c))". This file also uses "++n", "i++" and "((i++))". I will
send a v2 patch that normalizes these, open to discussion of course.
--
Philip Jägenstedt
^ permalink raw reply
* Re: how do you review auto-resolved files
From: Junio C Hamano @ 2012-02-21 21:19 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: git
In-Reply-To: <ji0vik$e48$1@dough.gmane.org>
"Neal Kreitzinger" <neal@rsss.com> writes:
> When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file
> changes that do not conflict on the same line(s).
>
> Technical Question: What are the recommended commands for reviewing the
> files that auto-resolved after a "merge"?
Imagine that you are the maintainer of the mainline and are reviewing the
work made on a side branch that you just merged, but pretend that the
contribution came as a patch instead. How would you assess the damage to
your mainline?
You would use "git show --first-parent $commit" for that.
And then look at what the sideline wanted to do to the old baseline:
git log -p $commit^..$commit
which would, unless the person who worked on the side branch did a shoddy
job describing his work, explain what the side branch wanted to achieve
and also _how_ it wanted to achieve it.
And then re-read the first "git show" output with that knowledge, together
with the knowledge you have on your mainline codebase, and decide if the
solution used by the side branch is still valid. If it makes sense, you
are done. If the advance in your mainline since the side branch forked
invalidated some assumption the side branch made (e.g. a helper function
the side branch used has changed its meaning, a helper function the side
branch changed its meaning gained more callsite on the mainline, etc.),
you have a semantic conflict that you would need to address.
It is unclear what exactly you consider "auto-resolve" in your message, so
I'd refrain from commenting on the "Philosophical" part, at least for now.
^ permalink raw reply
* [PATCH] cherry-pick: No advice to commit if --no-commit
From: Phil Hord @ 2012-02-21 21:05 UTC (permalink / raw)
To: Git List, Junio C Hamano, Ramkumar Ramachandra, Jonathan Nieder,
Phil Hord
Cc: Phil Hord
When cherry-pick fails it offers a helpful hint about how to
proceed. The hint tells the user how to do the cleanup
needed by the conflicted cherry-pick and finish the job after
conflict resolution. In case of cherry-pick --no-commit, the
hint goes too far. It tells the user to finish up with
'git commit'. That is not what this git-cherry-pick was
trying to do in the first place.
Restrict the hint in case of --no-commit to avoid giving this
extra advice.
Also, add a test verifying the reduced hint for the --no-commit
version of cherry-pick.
Signed-off-by: Phil Hord <hordp@cisco.com>
---
sequencer.c | 12 +++++++-----
t/t3507-cherry-pick-conflict.sh | 14 ++++++++++++++
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5fcbcb8..b7965e8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -123,7 +123,7 @@ static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
strbuf_release(&buf);
}
-static void print_advice(int show_hint)
+static void print_advice(int show_hint, struct replay_opts *opts)
{
char *msg = getenv("GIT_CHERRY_PICK_HELP");
@@ -138,10 +138,12 @@ static void print_advice(int show_hint)
return;
}
- if (show_hint)
+ if (show_hint) {
advise(_("after resolving the conflicts, mark the corrected paths\n"
- "with 'git add <paths>' or 'git rm <paths>'\n"
- "and commit the result with 'git commit'"));
+ "with 'git add <paths>' or 'git rm <paths>'"));
+ if (!opts->no_commit)
+ advise(_( "and commit the result with 'git commit'"));
+ }
}
static void write_message(struct strbuf *msgbuf, const char *filename)
@@ -423,7 +425,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
msg.subject);
- print_advice(res == 1);
+ print_advice(res == 1, opts);
rerere(opts->allow_rerere_auto);
} else {
if (!opts->no_commit)
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index ee1659c..0c81b3c 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -59,6 +59,20 @@ test_expect_success 'advice from failed cherry-pick' "
test_i18ncmp expected actual
"
+test_expect_success 'advice from failed cherry-pick --no-commit' "
+ pristine_detach initial &&
+
+ picked=\$(git rev-parse --short picked) &&
+ cat <<-EOF >expected &&
+ error: could not apply \$picked... picked
+ hint: after resolving the conflicts, mark the corrected paths
+ hint: with 'git add <paths>' or 'git rm <paths>'
+ EOF
+ test_must_fail git cherry-pick --no-commit picked 2>actual &&
+
+ test_i18ncmp expected actual
+"
+
test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
pristine_detach initial &&
test_must_fail git cherry-pick picked &&
--
1.7.9.267.gda172.dirty
^ permalink raw reply related
* Re: [PATCH] Ignore SIGPIPE when running a filter driver
From: Junio C Hamano @ 2012-02-21 20:58 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jehan Bing, git, gitster, j.sixt, peff
In-Reply-To: <20120221030150.GA31737@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
>> If a filter is not defined or if it fails, git behaves as if the filter
>> is a no-op passthru. However, if the filter exits before reading all
>> the content, and depending on the timing git, could be kill with
>> SIGPIPE instead.
>>
>> Ignore SIGPIPE while processing the filter to detect when it exits
>> early and fallback to using the unfiltered content.
>>
>> Signed-off-by: Jehan Bing <jehan@orb.com>
>
> For the benefit of the uninitiated ("how would ignoring an error help
> me detect an error?"): setting the SIGPIPE handler to SIG_IGN does not
> actually ignore the broken pipe condition but causes it to be reported
> as an I/O error, errno == EPIPE. That means instead of being killed
> by SIGPIPE, git gets to fall back to passthrough and report the
> filter's mistake.
Yes.
You could rephrase bit better to further clarify it, perhaps like this:
Ignore SIGPIPE when running a filter driver
If a filter is not defined or if it fails, git should behave as if the
filter is a no-op passthru.
However, if the filter exits before reading all the content, depending on
the timing, git could be killed with SIGPIPE when it tries to write to the
pipe connected to the filter.
Ignore SIGPIPE while processing the filter to give us a chance to check
the return value from a failed write, in order to detect and act on this
mode of failure in a more controlled way.
Signed-off-by: Jehan Bing <jehan@orb.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
although I think Jehan's original was already clear enough.
> So at least on POSIX-y platforms, this patch looks good to me. Thanks
> for writing it.
Thank you and Johannes for eyeballing and sanity checking.
Will queue.
^ permalink raw reply
* Re: [PATCH] completion: Allow dash as the first character for __git_ps1
From: Christian Hammerl @ 2012-02-21 20:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vty2jgbsh.fsf@alter.siamese.dyndns.org>
Yes, kind of.
It is more like PS1='\W$(__git_ps1 "-[ %s ]")'. And i am fixing it after each update on my machine. ;-)
Junio C Hamano <gitster@pobox.com> schrieb:
>Christian Hammerl <info@christian-hammerl.de> writes:
>
>> If the argument for `__git_ps1` begins with a dash, `printf` tries to
>> interpret it as an option which results in an error message.
>> The problem is solved by adding '--' before the argument to tell
>> `printf` to not interpret the following argument as an option.
>> Adding '--' directly to the argument does not help because the
>argument
>> is enclosed by double quotes.
>>
>> Signed-off-by: Christian Hammerl <info@christian-hammerl.de>
>> ---
>> contrib/completion/git-completion.bash | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash
>b/contrib/completion/git-completion.bash
>> index 91c7acb..61ff152 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -313,7 +313,7 @@ __git_ps1 ()
>> fi
>>
>> local f="$w$i$s$u"
>> - printf "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
>> + printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
>> fi
>> }
>
>Hmm, so you may be doing something like PS1='\W$(__git_ps1 "-%s")'?
>OK, and thanks.
--
Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.
^ 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