* Re: [PATCH] Correctly report corrupted objects
From: Junio C Hamano @ 2011-01-20 21:05 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: git, Will Palmer
In-Reply-To: <20110120201220.GD12418@atjola.homenet>
Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> The errno check added in commit 3ba7a06 "A loose object is not corrupt
> if it cannot be read due to EMFILE" only checked for whether errno is
> not ENOENT and thus incorrectly treated "no error" as an error
> condition.
>
> Because of that, it never reached the code path that would report that
> the object is corrupted and instead caused funny errors like:
>
> fatal: failed to read object 333c4768ce595793fdab1ef3a036413e2a883853: Success
>
> So we have to extend the check to cover the case in which the object
> file was successfully read, but its contents are corrupted.
Hmm, what is the exact code path that read_object() callchain fails to set
errno when it returns a NULL? It is unclear from the above description.
Is there a funny object replacement involved?
> Reported-by: Will Palmer <wmpalmer@gmail.com>
> Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> ---
> sha1_file.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 1cafdfa..d86a8db 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2141,7 +2141,7 @@ void *read_sha1_file_repl(const unsigned char *sha1,
> return data;
> }
>
> - if (errno != ENOENT)
> + if (errno && errno != ENOENT)
> die_errno("failed to read object %s", sha1_to_hex(sha1));
>
> /* die if we replaced an object with one that does not exist */
> --
> 1.7.4.rc2.18.gb20e9
^ permalink raw reply
* Re: [PATCH] Correctly report corrupted objects
From: Junio C Hamano @ 2011-01-20 21:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Björn Steinbrink, git, Will Palmer
In-Reply-To: <7vsjwnqoao.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
>
>> The errno check added in commit 3ba7a06 "A loose object is not corrupt
>> if it cannot be read due to EMFILE" only checked for whether errno is
>> not ENOENT and thus incorrectly treated "no error" as an error
>> condition.
>>
>> Because of that, it never reached the code path that would report that
>> the object is corrupted and instead caused funny errors like:
>>
>> fatal: failed to read object 333c4768ce595793fdab1ef3a036413e2a883853: Success
>>
>> So we have to extend the check to cover the case in which the object
>> file was successfully read, but its contents are corrupted.
>
> Hmm, what is the exact code path that read_object() callchain fails to set
> errno when it returns a NULL? It is unclear from the above description.
Ah, nevermind. I wasn't thinking. If for example unpack_sha1_header()
says the type bits are garbled, it will return -1 without having any
system calls failed up to that point (because the file itself was mmapped
and read correctly) and the calling unpack_sha1_file() will return NULL.
^ permalink raw reply
* Re: Parameter --color-words not documented for "git show"
From: Junio C Hamano @ 2011-01-20 21:25 UTC (permalink / raw)
To: Sebastian Pipping; +Cc: Thomas Rast, Git ML
In-Reply-To: <4D389E69.608@hartwork.org>
Sebastian Pipping <webmaster@hartwork.org> writes:
> On 01/20/11 21:27, Thomas Rast wrote:
>> Quote from the latter:
>>
>> This manual page describes only the most frequently used options.
>
> Okay. Is that a good a idea?
Yes; the alternative is to list everything.
> Is --abbrev-commit really used more
> frequently with "git show" than --color-words is?
I see this as a not-so-helpful-but-still-interesting question.
It depends on who you are, and if one wants to pick the most often used
ones, that selection may or may not coincide with _your_ usage pattern nor
mine. The original author apparently thought so, you seem to think
color-words is used a lot more often, and I personally think neither is
used often at all. So should we swap them, keep things as-is, or remove
both?
We obviously cannot take a poll to update the list every time a new user
starts using git, but it might make sense to review them every once in a
while.
^ permalink raw reply
* Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits
From: Junio C Hamano @ 2011-01-20 21:28 UTC (permalink / raw)
To: Thomas Rast
Cc: Nicolas Sebrecht, Jonathan Nieder, Matthieu Moy, git, gitster,
Kevin Ballard, Yann Dirson, Eric Raible, Johannes Schindelin
In-Reply-To: <201101202134.41911.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> (I for one have never accepted such a rebase; if the TODO only
> consists of noop, that means I made a mistake.)
Wouldn't that suggest us that if we were to do anything to this message it
would be a good idea to teach the user to "reset --hard" the branch if no
commits truly needs to be replayed on top of the onto-commit?
^ permalink raw reply
* Re: Creating remote branch called HEAD corrupts remote clones
From: Junio C Hamano @ 2011-01-20 21:43 UTC (permalink / raw)
To: Jeff King; +Cc: git, Stephen Kelly, KDE PIM
In-Reply-To: <20110120203840.GA11468@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Hmm. It seems like the symbolic ref is the culprit, not just HEAD. The
> HEAD thing is the most likely, of course, but I could do something like:
>
> git symbolic-ref refs/remotes/origin/convenient-alias \
> refs/remotes/origin/some-name-you-dont-like
Isn't it already wrong to do the above locally, in the sense that it is
equally wrong to do this?
git update-ref refs/remotes/origin/no-such-thing-exists-over-there
refs/heads/master
I agree that the symbolic ref is the real source of confusion in Stephen's
case (I admit that I was scratching my head chasing a non-existent bug in
transport_update_tracking_ref() before I realized what was happening), but
the usual (and the only) way for refs/remotes hierarchy to get a symbolic
ref is via a normal clone, and HEAD is the only name that can cause this
conflict. Creating a branch called HEAD (or FETCH_HEAD for that matter)
is infinitely more likely to be a mistake than being clever, I think.
> ... Maybe we should not follow symbolic
> refs during fetch. So if we are fetching the refspec "foo:bar", and the
> RHS "bar" is a symref, we should _not_ follow it, but instead just
> overwrite the symref with a regular ref.
>
> For pushing, one rule could be to allow pushing from a named symref, but
> not allow the matching rules to use a symref as a source....
I personally like this line of thought, especially as a thought experiment
to see what corner cases we could find, but I doubt I will be able to say
we covered all the corner cases with confidence without thinking long and
very hard. For now, I do not find this issue worth spending that kind of
deep thinking, especially when a lot simpler and easier to explain
workaround is available, but others may disagree and perfect your idea.
^ permalink raw reply
* Re: Creating remote branch called HEAD corrupts remote clones
From: Jeff King @ 2011-01-20 21:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Stephen Kelly, KDE PIM
In-Reply-To: <7vbp3bqmiy.fsf@alter.siamese.dyndns.org>
On Thu, Jan 20, 2011 at 01:43:17PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Hmm. It seems like the symbolic ref is the culprit, not just HEAD. The
> > HEAD thing is the most likely, of course, but I could do something like:
> >
> > git symbolic-ref refs/remotes/origin/convenient-alias \
> > refs/remotes/origin/some-name-you-dont-like
>
> Isn't it already wrong to do the above locally, in the sense that it is
> equally wrong to do this?
Probably. My argument in favor would be "that is what we are doing with
remotes/*/HEAD", but I think the logic is somewhat circular there.
Thinking on it more, yeah, if you want "convenient-alias", you are
probably better off to do:
git symbolic-ref refs/convenient-alias refs/remotes/origin/whatever
> git update-ref refs/remotes/origin/no-such-thing-exists-over-there
> refs/heads/master
Actually, don't we end up with that in the case of upstream deleting a
ref? Which isn't to say it isn't a stupid thing to do, but that we can
and do get into that state.
Thinking on it even more, I don't think we can cover all of the weird
"you have a ref named $foo and they suddenly created a ref named $foo"
push corner cases. There is no substitution for actual communication and
organization of ref names if you are going to be pushing along with
other people into a central repo. Because fundamentally the ref name is
the unique identifier, and matching refs during push tries to reconcile
matches based on those identifiers.
> I personally like this line of thought, especially as a thought experiment
> to see what corner cases we could find, but I doubt I will be able to say
> we covered all the corner cases with confidence without thinking long and
> very hard. For now, I do not find this issue worth spending that kind of
> deep thinking, especially when a lot simpler and easier to explain
> workaround is available, but others may disagree and perfect your idea.
Yeah, after reading your response and considering a bit, I think the
simple "don't make HEAD" thing (or at least "don't pull or push HEAD")
is a sane workaround.
-Peff
^ permalink raw reply
* Re: Parameter --color-words not documented for "git show"
From: Nicolas Sebrecht @ 2011-01-20 23:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sebastian Pipping, Thomas Rast, Git ML, Nicolas Sebrecht
In-Reply-To: <7vk4hzqnbx.fsf@alter.siamese.dyndns.org>
The 20/01/11, Junio C Hamano wrote:
> Sebastian Pipping <webmaster@hartwork.org> writes:
>
> > On 01/20/11 21:27, Thomas Rast wrote:
> >> Quote from the latter:
> >>
> >> This manual page describes only the most frequently used options.
> >
> > Okay. Is that a good a idea?
>
> Yes; the alternative is to list everything.
Would it be bad? I tend to think that a manual page is the good place to
list everything the program accepts as parameters and how to use them.
FMHO, Manual page is not where newcomers look to learn but it should
help everybody to find and understand all of the available options.
--
Nicolas Sebrecht
^ permalink raw reply
* Re: [PATCH] Sanity-ckeck config variable names
From: Jeff King @ 2011-01-20 23:22 UTC (permalink / raw)
To: Libor Pechacek; +Cc: git
In-Reply-To: <20110119141112.GD8034@fm.suse.cz>
On Wed, Jan 19, 2011 at 03:11:12PM +0100, Libor Pechacek wrote:
> Sanity-ckeck config variable names when adding and retrieving them.
>
> As a side effect code duplication between git_config_set_multivar and get_value
> (in builtin/config.c) was removed and the common functionality was placed in
> git_config_parse_key.
I think this is a good goal, but a few nits:
> +/* Auxiliary function to sanity-check and split the key into the section
> + * identifier and variable name.
> + *
> + * Returns 0 on success, 1 when there is an invalid character in the key and 2
> + * if there is no section name in the key.
Please switch these to -1 and -2, as we generally use negative integers
to indicate errors in library-ish function. I know you were just copying
git_config_set_multivar's error codes, but it is designed to return
straight to exit(), which makes it an exception.
Other than that, the code looks OK to me. However, it does cause
t1300.85 to fail. The problem is that the test is using these bogus
names to check that "git -c" works. While it does technically work now
to say "git -c foo=bar config foo" (which your patch breaks), I don't
think that is a useful behavior in the real world, since no actual
config options exist without a section name. So yes, you can "git -c" a
non-sectioned variable, but why would you want to?
So I think it probably makes sense to squash this in:
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index d0e5546..3e79c37 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -876,11 +876,10 @@ test_expect_success 'check split_cmdline return' "
"
test_expect_success 'git -c "key=value" support' '
- test "z$(git -c name=value config name)" = zvalue &&
test "z$(git -c core.name=value config core.name)" = zvalue &&
- test "z$(git -c CamelCase=value config camelcase)" = zvalue &&
- test "z$(git -c flag config --bool flag)" = ztrue &&
- test_must_fail git -c core.name=value config name
+ test "z$(git -c foo.CamelCase=value config foo.camelcase)" = zvalue &&
+ test "z$(git -c foo.flag config --bool foo.flag)" = ztrue &&
+ test_must_fail git -c name=value config core.name
'
test_done
and a note to the commit message like:
This breaks a test in t1300 which used invalid section-less keys in
the tests for "git -c". However, allowing such names there was
useless, since there was no way to set them via config file, and no
part of git actually tried to use section-less keys. This patch
updates the test to use more realistic examples.
-Peff
^ permalink raw reply related
* Re: Parameter --color-words not documented for "git show"
From: Jeff King @ 2011-01-20 23:34 UTC (permalink / raw)
To: Nicolas Sebrecht; +Cc: Junio C Hamano, Sebastian Pipping, Thomas Rast, Git ML
In-Reply-To: <20110120231649.GC14184@vidovic>
On Fri, Jan 21, 2011 at 12:16:49AM +0100, Nicolas Sebrecht wrote:
> The 20/01/11, Junio C Hamano wrote:
> > Sebastian Pipping <webmaster@hartwork.org> writes:
> >
> > > On 01/20/11 21:27, Thomas Rast wrote:
> > >> Quote from the latter:
> > >>
> > >> This manual page describes only the most frequently used options.
> > >
> > > Okay. Is that a good a idea?
> >
> > Yes; the alternative is to list everything.
>
> Would it be bad? I tend to think that a manual page is the good place to
> list everything the program accepts as parameters and how to use them.
> FMHO, Manual page is not where newcomers look to learn but it should
> help everybody to find and understand all of the available options.
The problem is that we have a bazillion diff options that appear in many
manpages, so you are stuck with one of:
1. repeat them all in each manpage (usually via some automagic
include), which dwarfs the original content, and makes it hard for
users to see subtle differences between commands
2. Say "this describes only the most frequently used options", which
leaves the user wondering which infrequently used options exist.
3. Say "we also take diff options, and you can find out more about
diff options in git-diff(1)." This at least points the user in the
right direction, but you can't search for "--color-words" in the
page.
4. Do (3), but also list the all (or common) diff options in a succint
list without descriptions, and refer the user to git-diff(1). Then
they can grep if they like, and while they won't get the immediate
answer, they will get referred to the right place.
As you can probably guess, I favor option (4), though we already do (3)
in some places.
-Peff
^ permalink raw reply
* Re: Creating remote branch called HEAD corrupts remote clones
From: Felipe Contreras @ 2011-01-20 23:52 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Stephen Kelly, KDE PIM
In-Reply-To: <20110120215456.GB11468@sigill.intra.peff.net>
Hi,
On Thu, Jan 20, 2011 at 11:54 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 20, 2011 at 01:43:17PM -0800, Junio C Hamano wrote:
>> I personally like this line of thought, especially as a thought experiment
>> to see what corner cases we could find, but I doubt I will be able to say
>> we covered all the corner cases with confidence without thinking long and
>> very hard. For now, I do not find this issue worth spending that kind of
>> deep thinking, especially when a lot simpler and easier to explain
>> workaround is available, but others may disagree and perfect your idea.
>
> Yeah, after reading your response and considering a bit, I think the
> simple "don't make HEAD" thing (or at least "don't pull or push HEAD")
> is a sane workaround.
I don't fully understand the issue, so excuse me if this is totally
wrong, but wouldn't a rule like 'you can't create a branch for which
there's already a symbolic ref' do the trick?
--
Felipe Contreras
^ permalink raw reply
* impact on git of changing linux o/s system-date frequently
From: Neal Kreitzinger @ 2011-01-21 0:01 UTC (permalink / raw)
To: git
What are the implications of linux o/s system-date changes on git?
We plan on having test VM's on which the linux o/s system-date (linux "date"
command) is changed several times to test time lapse scenarios, e.g. go
forward 30 days in the future. We use git to pull updates from the
development machine to mirror repos and cloned repos on the test VM, and to
create test areas (local repos) on the test VM. We do not use git-push or
git-commit on the test VM at this time.
v/r,
Neal
^ permalink raw reply
* Re: Parameter --color-words not documented for "git show"
From: Sebastian Pipping @ 2011-01-21 0:05 UTC (permalink / raw)
To: Jeff King; +Cc: Nicolas Sebrecht, Junio C Hamano, Thomas Rast, Git ML
In-Reply-To: <20110120233429.GB9442@sigill.intra.peff.net>
On 01/21/11 00:34, Jeff King wrote:
>>> Yes; the alternative is to list everything.
>>
>> Would it be bad? I tend to think that a manual page is the good place to
>> list everything the program accepts as parameters and how to use them.
>> FMHO, Manual page is not where newcomers look to learn but it should
>> help everybody to find and understand all of the available options.
>
> The problem is that we have a bazillion diff options that appear in many
> manpages, so you are stuck with one of:
>
> 1. repeat them all in each manpage (usually via some automagic
> include), which dwarfs the original content, and makes it hard for
> users to see subtle differences between commands
>
> 2. Say "this describes only the most frequently used options", which
> leaves the user wondering which infrequently used options exist.
>
> 3. Say "we also take diff options, and you can find out more about
> diff options in git-diff(1)." This at least points the user in the
> right direction, but you can't search for "--color-words" in the
> page.
>
> 4. Do (3), but also list the all (or common) diff options in a succint
> list without descriptions, and refer the user to git-diff(1). Then
> they can grep if they like, and while they won't get the immediate
> answer, they will get referred to the right place.
>
> As you can probably guess, I favor option (4), though we already do (3)
> in some places.
I agree with Thomas here. (1) is the only option I find acceptable,
personally. If you'd rather not do that, then at least know I now.
Great to have --color-words around btw.
Best,
Sebastian
^ permalink raw reply
* Re: [PATCH] Sanity-ckeck config variable names
From: Jeff King @ 2011-01-21 0:06 UTC (permalink / raw)
To: Libor Pechacek; +Cc: git
In-Reply-To: <20110120232232.GA9442@sigill.intra.peff.net>
On Thu, Jan 20, 2011 at 06:22:32PM -0500, Jeff King wrote:
> Other than that, the code looks OK to me.
Actually, I take this back.
Doesn't this hunk:
> @@ -168,10 +167,6 @@ static int get_value(const char *key_, const char *regex_)
> }
>
> key = xstrdup(key_);
> - for (tl=key+strlen(key)-1; tl >= key && *tl != '.'; --tl)
> - *tl = tolower(*tl);
> - for (tl=key; *tl && *tl != '.'; ++tl)
> - *tl = tolower(*tl);
Mean that regexp keys no longer get downcased properly? I.e.,
git config Foo.value true
git config --get-regexp 'foo.*'
git config --get-regexp 'Foo.*'
used to work for both lookups, but now fails for the second one?
The problem is that your git_config_parse_key handles the downcasing for
the non-regexp case, but it is not called (for obvious reasons) in the
regexp case.
-Peff
^ permalink raw reply
* Re: Parameter --color-words not documented for "git show"
From: Jeff King @ 2011-01-21 0:20 UTC (permalink / raw)
To: Sebastian Pipping; +Cc: Nicolas Sebrecht, Junio C Hamano, Thomas Rast, Git ML
In-Reply-To: <4D38CDC4.6010803@hartwork.org>
On Fri, Jan 21, 2011 at 01:05:24AM +0100, Sebastian Pipping wrote:
> > The problem is that we have a bazillion diff options that appear in many
> > manpages, so you are stuck with one of:
> >
> > 1. repeat them all in each manpage (usually via some automagic
> > include), which dwarfs the original content, and makes it hard for
> > users to see subtle differences between commands
> >
> > 2. Say "this describes only the most frequently used options", which
> > leaves the user wondering which infrequently used options exist.
> >
> > 3. Say "we also take diff options, and you can find out more about
> > diff options in git-diff(1)." This at least points the user in the
> > right direction, but you can't search for "--color-words" in the
> > page.
> >
> > 4. Do (3), but also list the all (or common) diff options in a succint
> > list without descriptions, and refer the user to git-diff(1). Then
> > they can grep if they like, and while they won't get the immediate
> > answer, they will get referred to the right place.
> >
> > As you can probably guess, I favor option (4), though we already do (3)
> > in some places.
>
> I agree with Thomas here. (1) is the only option I find acceptable,
> personally. If you'd rather not do that, then at least know I now.
> Great to have --color-words around btw.
I'm curious why (4) doesn't work for you. I assumed you came to the
problem by one of:
- you wanted to know which options "git show" had, so you looked in
the manpage. Nothing told you about "--color-words", nor referred
you to a list of diff options. With (4), you would find that it
accepted all diff options, and then go read the list of diff options
(if you weren't already familiar with it).
- you knew about --color-words, and wondered if "git show" supported
it. In the current case, searching the page turns up nothing. In
option (4), a search would find it (with a reference to diff options
if you wanted more details).
The downside is that you sometimes have to be referred. The upside to me
is that it becomes explicit that there is a concept of "diff options"
that you can look up easily and which we can refer to easily in other
parts of the manual. That helps establish a mental model of how git's
options work.
So is it just that being referred is annoying, or something else?
-Peff
^ permalink raw reply
* Re: [PATCH] Documentation fixes in git-config
From: Jeff King @ 2011-01-21 0:27 UTC (permalink / raw)
To: Libor Pechacek; +Cc: git
In-Reply-To: <20110119141401.GE8034@fm.suse.cz>
On Wed, Jan 19, 2011 at 03:14:01PM +0100, Libor Pechacek wrote:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ff7c225..0f23bc7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -12,8 +12,9 @@ The configuration variables are used by both the git plumbing
> and the porcelains. The variables are divided into sections, wherein
> the fully qualified variable name of the variable itself is the last
> dot-separated segment and the section name is everything before the last
> -dot. The variable names are case-insensitive and only alphanumeric
> -characters are allowed. Some variables may appear multiple times.
> +dot. The variable names are case-insensitive, only alphanumeric
> +characters and '-' are allowed and must start with an alphabetic character.
> +Some variables may appear multiple times.
The intent of the change looks fine, but your sentence doesn't quite
parse to me (to be fair, the problem is in the one you are replacing,
but adding the third clause makes it even more confusing). How about:
The variables names are case-insensitive, allow only alphanumeric
characters and '-', and must start with an alphabetic character.
> --get-regexp::
> Like --get-all, but interprets the name as a regular expression.
> - Also outputs the key names.
> + Regular expression matching is case sensitive in all parts of the key,
> + therefore make sure your pattern matches lower case letters in section
> + and variable names. Also outputs the key names.
That is only true because of the breakage in your first patch. Without
your patch, both of these work:
git config --get-regexp 'Foo.*'
git config --get-regexp 'foo.*'
That being said, the downcasing is extremely naive for regexps, and you
should try to match the canonical name. The current downcasing behavior
should probably stay for historical reasons, but is not well thought-out
(it may even be accidental). Perhaps we should therefore explain it in
those terms:
Regular expression matching is case-sensitive and done against a
canonicalized version of the key in which section and variable names
are lowercased, but subsection names are not. For historical reasons,
some simple regular expressions are lower-cased before matching
(everything before the first dot and after the last dot), which makes
things like "Core.*' work.
I dunno. Maybe we should just declare "Core.*' to be broken, and anybody
who was relying on it is wrong.
-Peff
^ permalink raw reply
* Re: Parameter --color-words not documented for "git show"
From: Sebastian Pipping @ 2011-01-21 0:27 UTC (permalink / raw)
To: Jeff King; +Cc: Nicolas Sebrecht, Junio C Hamano, Thomas Rast, Git ML
In-Reply-To: <20110121002020.GA7874@sigill.intra.peff.net>
On 01/21/11 01:20, Jeff King wrote:
>> I agree with Thomas here. (1) is the only option I find acceptable,
>> personally. If you'd rather not do that, then at least know I now.
>> Great to have --color-words around btw.
>
> I'm curious why (4) doesn't work for you. I assumed you came to the
> problem by one of:
>
> - you wanted to know which options "git show" had, so you looked in
> the manpage. Nothing told you about "--color-words", nor referred
> you to a list of diff options. With (4), you would find that it
> accepted all diff options, and then go read the list of diff options
> (if you weren't already familiar with it).
>
> - you knew about --color-words, and wondered if "git show" supported
> it. In the current case, searching the page turns up nothing. In
> option (4), a search would find it (with a reference to diff options
> if you wanted more details).
>
> The downside is that you sometimes have to be referred. The upside to me
> is that it becomes explicit that there is a concept of "diff options"
> that you can look up easily and which we can refer to easily in other
> parts of the manual. That helps establish a mental model of how git's
> options work.
>
> So is it just that being referred is annoying, or something else?
Actually that approach is perfect. I misunderstood (4) on the first
read somehow. Really not my day today, sorry. I would love to see you
push (4) forward.
Best,
Sebastian
^ permalink raw reply
* [PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"
From: Jonathan Nieder @ 2011-01-21 0:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, git, Kevin Ballard, Yann Dirson, Eric Raible
In-Reply-To: <7vy66fqoji.fsf@alter.siamese.dyndns.org>
The hints in the current "instruction sheet" template look like so:
# Rebase 3f14246..a1d7e01 onto 3f14246
#
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
# x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
#
# If you remove a line here THAT COMMIT WILL BE LOST.
# However, if you remove everything, the rebase will be aborted.
#
This does not make it clear that the format of each line is
<insn> <commit id> <explanatory text that will be printed>
but the reader will probably infer that from the automatically
generated pick examples above it.
What about the "exec" instruction? By analogy, I might imagine that
the format of that line is "exec <command> <explanatory text>", and
the "x <cmd>" hint does not address that question (at first I read it
as taking an argument <cmd> that is the name of a shell). Meanwhile,
the mention of <cmd> makes the hints harder to scan as a table.
So remove the <cmd> and add some words to remind the reader that
"exec" runs a command named by the rest of the line. To make room, it
is left to the manpage to explain that that command is run using
$SHELL and that nonzero status from that command will pause the
rebase.
Wording from Junio.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
> If we wanted to be more helpful, perhaps s/(see below)/specified on the
> rest of the line/ should be sufficient without adding extra lines.
Sounds good.
git-rebase--interactive.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a5ffd9a..a18c9b1 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1021,7 +1021,7 @@ first and then run 'git rebase --continue' again."
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
-# x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
+# x, exec = run command specified on the rest of the line
#
# If you remove a line here THAT COMMIT WILL BE LOST.
# However, if you remove everything, the rebase will be aborted.
--
1.7.4.rc2
^ permalink raw reply related
* Re: Parameter --color-words not documented for "git show"
From: Junio C Hamano @ 2011-01-21 6:08 UTC (permalink / raw)
To: Jeff King; +Cc: Nicolas Sebrecht, Sebastian Pipping, Thomas Rast, Git ML
In-Reply-To: <20110120233429.GB9442@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The problem is that we have a bazillion diff options that appear in many
> manpages, so you are stuck with one of:
>
> 1. repeat them all in each manpage (usually via some automagic
> include), which dwarfs the original content, and makes it hard for
> users to see subtle differences between commands
>
> 2. Say "this describes only the most frequently used options", which
> leaves the user wondering which infrequently used options exist.
>
> 3. Say "we also take diff options, and you can find out more about
> diff options in git-diff(1)." This at least points the user in the
> right direction, but you can't search for "--color-words" in the
> page.
>
> 4. Do (3), but also list the all (or common) diff options in a succint
> list without descriptions, and refer the user to git-diff(1). Then
> they can grep if they like, and while they won't get the immediate
> answer, they will get referred to the right place.
>
> As you can probably guess, I favor option (4), though we already do (3)
> in some places.
We attempt to do 1 to solve it "nicely" in some manual pages, and indeed
there are many "exclude this option from the command X's manpage" magic
that causes the problem of subtle differences you mentioned (which I
agree).
One complication in either 3 or 4 is that they sometimes need to be
accompanied with "... except these diff options do not make sense in the
context of this command, so they are no-op". That is probably a price
worth paying to be more helpful than 2 is.
^ permalink raw reply
* Re: [PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"
From: Matthieu Moy @ 2011-01-21 6:59 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, git, Kevin Ballard, Yann Dirson, Eric Raible
In-Reply-To: <20110121003624.GB23139@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
> -# x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
> +# x, exec = run command specified on the rest of the line
I don't think dropping "shell" is a good idea. In this context,
"command" could mean "one of pick/fixup/squash/...", a Git command,
and at last, an arbitrary line of shell.
I agree that my "shell command" wording was confusing too, but maybe
just adding "using the shell" at the end of line would do it.
Otherwise, I prefered the "see below + 2 lines explanation" proposal
above.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits
From: Johannes Schindelin @ 2011-01-21 7:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Nicolas Sebrecht, Jonathan Nieder, Matthieu Moy, git,
Kevin Ballard, Yann Dirson, Eric Raible
In-Reply-To: <7vfwsnqn8c.fsf@alter.siamese.dyndns.org>
Hi,
On Thu, 20 Jan 2011, Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
>
> > (I for one have never accepted such a rebase; if the TODO only
> > consists of noop, that means I made a mistake.)
>
> Wouldn't that suggest us that if we were to do anything to this message
> it would be a good idea to teach the user to "reset --hard" the branch
> if no commits truly needs to be replayed on top of the onto-commit?
The important difference between rebase -i && noop on the one, and reset
--hard on the other hand is that the latter is completely unsafe. I mean,
utterly completely super-unsafe. And I say that because _this here
developer_ who is not exactly a Git noob lost stuff that way.
rebase -i checks that all is well and we could come back to the current
status later if we realized that things went horribly wrong.
reset --hard does not do that. No safety net. No reflog. Nada.
Hth,
Dscho
^ permalink raw reply
* Re: impact on git of changing linux o/s system-date frequently
From: David Brown @ 2011-01-21 7:13 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: git
In-Reply-To: <ihaie3$63g$1@dough.gmane.org>
On Thu, Jan 20 2011, Neal Kreitzinger wrote:
> What are the implications of linux o/s system-date changes on git?
>
> We plan on having test VM's on which the linux o/s system-date (linux "date"
> command) is changed several times to test time lapse scenarios, e.g. go
> forward 30 days in the future. We use git to pull updates from the
> development machine to mirror repos and cloned repos on the test VM, and to
> create test areas (local repos) on the test VM. We do not use git-push or
> git-commit on the test VM at this time.
Without making commits on the machine, there won't be any history being
created with misordered dates, so there shouldn't be any issues there.
The index should also be pretty resiliant to time warping as well, and
if you aren't modifying any of the git managed files, I don't think you
can provoke anything.
If you're doing edits of files, there's a slight possibility the edited
version could end up with the same ctime/mtime as the cache in the
index, and git would miss the change. Again, it doesn't sound like you
are doing this.
David
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* [PATCH] Documentation: suggest "reset --keep" to undo a commit
From: Jonathan Nieder @ 2011-01-21 7:37 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Thomas Rast, Nicolas Sebrecht, Matthieu Moy, git,
Kevin Ballard, Yann Dirson, Eric Raible, Christian Couder
In-Reply-To: <alpine.DEB.1.00.1101210801210.15247@pacific.mpi-cbg.de>
When one's only goal is to move from one commit to another, reset
--keep is simply better than reset --hard, since it preserves local
changes in the index and worktree when easy and errors out without
doing anything when not. Update the two "how to remove commits"
examples in this vein. "reset --hard" is still explained in a later
example about cleaning up during a merge.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Johannes Schindelin wrote:
> rebase -i checks that all is well and we could come back to the current
> status later if we realized that things went horribly wrong.
>
> reset --hard does not do that. No safety net. No reflog. Nada.
Right. I think we should encourage people to use "reset --keep" more
often. (In general. The particular "rebase to pull" example just
mentioned is less obvious.)
Documentation/git-reset.txt | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index fd72976..1f13a1e 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -148,7 +148,7 @@ Undo a commit, making it a topic branch::
+
------------
$ git branch topic/wip <1>
-$ git reset --hard HEAD~3 <2>
+$ git reset --keep HEAD~3 <2>
$ git checkout topic/wip <3>
------------
+
@@ -163,7 +163,7 @@ Undo commits permanently::
+
------------
$ git commit ...
-$ git reset --hard HEAD~3 <1>
+$ git reset --keep HEAD~3 <1>
------------
+
<1> The last three commits (HEAD, HEAD^, and HEAD~2) were bad
--
1.7.4.rc2
^ permalink raw reply related
* Re: [PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"
From: Jonathan Nieder @ 2011-01-21 7:47 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, git, Kevin Ballard, Yann Dirson, Eric Raible
In-Reply-To: <vpqr5c6zqrh.fsf@bauges.imag.fr>
Matthieu Moy wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> -# x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
>> +# x, exec = run command specified on the rest of the line
>
> I don't think dropping "shell" is a good idea. In this context,
> "command" could mean "one of pick/fixup/squash/...", a Git command,
> and at last, an arbitrary line of shell.
Hmm. I suppose
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
# x, exec = run command (the rest of the line) using shell
would do?
Sorry to take so long to get this right.
^ permalink raw reply
* Re: [PATCH] Sanity-ckeck config variable names
From: Libor Pechacek @ 2011-01-21 10:02 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20110120232232.GA9442@sigill.intra.peff.net>
On Thu 20-01-11 18:22:32, Jeff King wrote:
> > + * Returns 0 on success, 1 when there is an invalid character in the key and 2
> > + * if there is no section name in the key.
>
> Please switch these to -1 and -2, as we generally use negative integers
> to indicate errors in library-ish function.
Done.
> Other than that, the code looks OK to me. However, it does cause
> t1300.85 to fail.
Thanks for catching it, added your changes to the patch and tested. I didn't
notice the test suite. :)
On Thu 20-01-11 19:06:29, Jeff King wrote:
> Doesn't this hunk:
>
> > @@ -168,10 +167,6 @@ static int get_value(const char *key_, const char *regex_)
> > }
> >
> > key = xstrdup(key_);
> > - for (tl=key+strlen(key)-1; tl >= key && *tl != '.'; --tl)
> > - *tl = tolower(*tl);
> > - for (tl=key; *tl && *tl != '.'; ++tl)
> > - *tl = tolower(*tl);
>
> Mean that regexp keys no longer get downcased properly? I.e.,
>
> git config Foo.value true
> git config --get-regexp 'foo.*'
> git config --get-regexp 'Foo.*'
>
> used to work for both lookups, but now fails for the second one?
After thinking about it I added the code back. More on it in the
"documentation fix" thread.
Thanks for your kind guidance.
Libor
--
Libor Pechacek
SUSE L3 Team, Prague
^ permalink raw reply
* Re: Parameter --color-words not documented for "git show"
From: Maaartin @ 2011-01-21 10:08 UTC (permalink / raw)
To: Jeff King
Cc: Nicolas Sebrecht, Junio C Hamano, Sebastian Pipping, Thomas Rast,
Git ML
In-Reply-To: <20110120233429.GB9442@sigill.intra.peff.net>
On 11-01-21 00:34, Jeff King wrote:
> On Fri, Jan 21, 2011 at 12:16:49AM +0100, Nicolas Sebrecht wrote:
> 4. Do (3), but also list the all (or common) diff options in a succint
> list without descriptions, and refer the user to git-diff(1). Then
> they can grep if they like, and while they won't get the immediate
> answer, they will get referred to the right place.
>
> As you can probably guess, I favor option (4), though we already do (3)
> in some places.
I also favor (4), for the following reasons:
- sometimes you want to read the whole manpage, e.g., it's good for
beginners to get feeling what is it all about.
- repeated information makes the page too long and reading too boring.
- too long manpages may scare beginners.
Maybe there could be sort-of extended manpage, containing everything,
but it would need some markup beyond the capabilities of a terminal (a
grayed or collapsed area in html, or whatever). However, this could be a
lot of additional work, and I don't claim it should be done, just an idea.
^ 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