* [PATCH v2 3/3] push: further simplify the logic to assign rejection status
From: Junio C Hamano @ 2013-01-22 6:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358836230-9197-1-git-send-email-gitster@pobox.com>
Instead of using deeply nested if/else statements, first decide what
rejection status we would get if this push weren't forced, and then
assign the rejection reason to the ref->status field and flip the
ref->forced_update field when we forced a push for a ref that indeed
required forcing.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The first one mistakenly changed the semantics and reported a
forced push even when the push was done with useless and
unnecessary --force option (e.g. the update was properly
fast-forwarding but --force was given from the command line).
This fixes it.
remote.c | 40 +++++++++++++++-------------------------
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/remote.c b/remote.c
index c991915..af2136d 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,32 +1318,22 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
*/
if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
- if (!prefixcmp(ref->name, "refs/tags/")) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
- continue;
- }
+ int status = 0;
+
+ if (!prefixcmp(ref->name, "refs/tags/"))
+ status = REF_STATUS_REJECT_ALREADY_EXISTS;
+ else if (!has_sha1_file(ref->old_sha1))
+ status = REF_STATUS_REJECT_FETCH_FIRST;
+ else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
+ !lookup_commit_reference_gently(ref->new_sha1, 1))
+ status = REF_STATUS_REJECT_NEEDS_FORCE;
+ else if (!ref_newer(ref->new_sha1, ref->old_sha1))
+ status = REF_STATUS_REJECT_NONFASTFORWARD;
+
+ if (!force_ref_update)
+ ref->status = status;
+ else if (status)
ref->forced_update = 1;
- } else if (!has_sha1_file(ref->old_sha1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_FETCH_FIRST;
- continue;
- }
- ref->forced_update = 1;
- } else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
- !lookup_commit_reference_gently(ref->new_sha1, 1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
- continue;
- }
- ref->forced_update = 1;
- } else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
- continue;
- }
- ref->forced_update = 1;
- }
}
}
}
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-22 6:44 UTC (permalink / raw)
To: Chris Rorvick
Cc: Jeff King, Max Horn, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <CAEUsAPYaK3PP67fc89-J3a83wzYcmu7HRyh7y1Kctg6d166LEQ@mail.gmail.com>
Chris Rorvick <chris@rorvick.com> writes:
> I agree with everything above. I just don't understand why reverting
> the "already exists" behavior for non-commit-ish objects was a
> prerequisite to fixing this.
Because it is a regression. People who did not force such a push
did not get "already exists", but with your patch they do.
By reverting the wrong message so that we get the old wrong message
instead, people will only have to deal with an already known
breakage; a known devil is better than an unknown new devil (or an
unknown angel).
When a change that brings in a regression and an improvement at the
same time, it does not matter what the improvement is; we fix the
regression first as soon as safely possible and we then attempt to
resurrect and polish the improvement.
^ permalink raw reply
* Re: [PATCH 0/3] Finishing touches to "push" advises
From: Junio C Hamano @ 2013-01-22 7:26 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358836230-9197-1-git-send-email-gitster@pobox.com>
As far as I am concerned, I am pretty much done with this topic, at
least for now. Of course if there are bugreports I'll try to help
resolving them, but I do not expect myself adding new object-type
based policy decision to this codepath.
The call the updated call makes to ref_newer() no longer feeds
certain combinations to the function, because the NULL-ness of the
old and commit-ness of both are checked before making a call.
I notice that builtin/remote.c has another callsite for ref_newer().
Although I didn't look at the code, I think it is trying to see if
the branch can be pushed as a fast-forward to the remote (or the
remote tip moved since you started building on top of it).
It probably makes sense to refactor the logic that is run per-ref in
the loop in the set_ref_status_for_push() function into a new helper
function, inline ref_newer() there, and have the remaining callers
of ref_newer() to use that new helper function, which knows the new
rules such as "refs/tags/ cannot be replaced with anything without
force".
^ permalink raw reply
* Re: [RFC/PATCH] add: warn when -u or -A is used without filepattern
From: Matthieu Moy @ 2013-01-22 7:39 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, gitster, Eric James Michael Ritz, Tomas Carnecky
In-Reply-To: <20130121222248.GA3586@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Would it be possible to make this conditional on cwd not being at the
> toplevel (the case where "git add -u :/" and "git add -u ." have
> different behavior)? E.g.,
>
> static const char *here[2] = { ".", NULL };
> if (prefix)
> warning(...);
I thought about this too, after writting the patch. Actually, I still I
it makes sense to warn even from the toplevel, since the point is to
teach people to stop using pathless "git add -u" for a while, so I'd say
it's easier to teach this in every condition. OTOH, the next step
(forbidding pathless "git add -u") can still allow it from the toplevel
to minimize the pain.
But I'm starting to be convinced ;-).
Any other thought on the question?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH v3] Enable minimal stat checking
From: Robin Rosenberg @ 2013-01-22 7:49 UTC (permalink / raw)
To: git, Junio C Hamano; +Cc: j sixt, Shawn Pearce, Robin Rosenberg
In-Reply-To: <7v4niblhr6.fsf@alter.siamese.dyndns.org>
Specifically the fields uid, gid, ctime, ino and dev are set to zero
by JGit. Other implementations, eg. Git in cygwin are allegedly also
somewhat incompatible with Git For Windows and on *nix platforms
the resolution of the timestamps may differ.
Any stat checking by git will then need to check content, which may
be very slow, particularly on Windows. Since mtime and size
is typically enough we should allow the user to tell git to avoid
checking these fields if they are set to zero in the index.
This change introduces a core.checkstat config option where the
the user can select to check all fields (default), or just size
and the whole second part of mtime (minimal).
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
Documentation/config.txt | 6 ++++++
cache.h | 1 +
config.c | 8 ++++++++
environment.c | 1 +
read-cache.c | 28 ++++++++++++++++------------
5 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5809e0..47c213d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -235,6 +235,12 @@ core.trustctime::
crawlers and some backup systems).
See linkgit:git-update-index[1]. True by default.
+core.checkstat::
+ Determines which stat fields to match between the index
+ and work tree. The user can set this to 'default' or
+ 'minimal'. Default (or explicitly 'default'), is to check
+ all fields, including the sub-second part of mtime and ctime.
+
core.quotepath::
The commands that output paths (e.g. 'ls-files',
'diff'), when not given the `-z` option, will quote
diff --git a/cache.h b/cache.h
index c257953..ab20c4d 100644
--- a/cache.h
+++ b/cache.h
@@ -536,6 +536,7 @@ extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
/* Environment bits from configuration mechanism */
extern int trust_executable_bit;
extern int trust_ctime;
+extern int check_stat;
extern int quote_path_fully;
extern int has_symlinks;
extern int minimum_abbrev, default_abbrev;
diff --git a/config.c b/config.c
index 7b444b6..2b58c75 100644
--- a/config.c
+++ b/config.c
@@ -566,6 +566,14 @@ static int git_default_core_config(const char *var, const char *value)
trust_ctime = git_config_bool(var, value);
return 0;
}
+ if (!strcmp(var, "core.statinfo")) {
+ if (!strcasecmp(value, "default"))
+ check_stat = 1;
+ else if (!strcasecmp(value, "minimal"))
+ check_stat = 0;
+ else
+ die_bad_config(var);
+ }
if (!strcmp(var, "core.quotepath")) {
quote_path_fully = git_config_bool(var, value);
diff --git a/environment.c b/environment.c
index 85edd7f..e828b37 100644
--- a/environment.c
+++ b/environment.c
@@ -13,6 +13,7 @@
int trust_executable_bit = 1;
int trust_ctime = 1;
+int check_stat = 1;
int has_symlinks = 1;
int minimum_abbrev = 4, default_abbrev = 7;
int ignore_case;
diff --git a/read-cache.c b/read-cache.c
index fda78bc..23db681 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -197,21 +197,25 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
}
if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
changed |= MTIME_CHANGED;
- if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
- changed |= CTIME_CHANGED;
+ if (trust_ctime ? check_stat : trust_ctime/*false*/)
+ if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
+ changed |= CTIME_CHANGED;
#ifdef USE_NSEC
- if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
+ if (check_stat && ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
changed |= MTIME_CHANGED;
- if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
- changed |= CTIME_CHANGED;
+ if (trust_ctime ? check_stat : trust_ctime/*false*/)
+ if (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
+ changed |= CTIME_CHANGED;
#endif
- if (ce->ce_uid != (unsigned int) st->st_uid ||
- ce->ce_gid != (unsigned int) st->st_gid)
- changed |= OWNER_CHANGED;
- if (ce->ce_ino != (unsigned int) st->st_ino)
- changed |= INODE_CHANGED;
+ if (check_stat) {
+ if (ce->ce_uid != (unsigned int) st->st_uid ||
+ ce->ce_gid != (unsigned int) st->st_gid)
+ changed |= OWNER_CHANGED;
+ if (ce->ce_ino != (unsigned int) st->st_ino)
+ changed |= INODE_CHANGED;
+ }
#ifdef USE_STDEV
/*
@@ -219,8 +223,8 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
* clients will have different views of what "device"
* the filesystem is on
*/
- if (ce->ce_dev != (unsigned int) st->st_dev)
- changed |= INODE_CHANGED;
+ if (check_stat && ce->ce_dev != (unsigned int) st->st_dev)
+ changed |= INODE_CHANGED;
#endif
if (ce->ce_size != (unsigned int) st->st_size)
--
1.8.1.337.g6672977.dirty
^ permalink raw reply related
* Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer
From: Jonathan Nieder @ 2013-01-22 7:54 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-2-git-send-email-drafnel@gmail.com>
Hi,
Brandon Casey wrote:
> --- a/sequencer.c
> +++ b/sequencer.c
[...]
> @@ -1042,13 +1041,8 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
Git is checking if (sb->buf) ends with a "Signed-off-by:" style
line. If it doesn't, it will need to add an extra blank line
before adding a new sign-off.
First (snipped), it seeks back two newlines from the end and then
forward to the next non-newline character, so (buf + i) is at the
start of the last line of (the interesting part of) sb. Now:
> for (; i < len; i = k) {
> for (k = i; k < len && buf[k] != '\n'; k++)
> ; /* do nothing */
> k++;
(buf + k) points to the end of this line.
> - if ((buf[k] == ' ' || buf[k] == '\t') && !first)
> - continue;
This is always the first line examined, so this "continue" never
triggers.
> -
> - first = 0;
> -
> for (j = 0; i + j < len; j++) {
If the line matches /^[[:alnum:]-]*:/, it passes and git moves on to
the (nonexistent) next line. Otherwise, it fails.
Do I understand correctly? If so, this patch should be a no-op, which
is good, I guess.
But in that case, couldn't this function be made much simpler? As far
as I can tell, all the function needs to do is the following:
1. Find the last line.
2. Check if it is blank or matches /^[[:alnum:]-]*:/
3. There is no step 3. That's it.
In other words, something like:
const char *eol, *p;
/* End of line */
eol = memrchr(sb->buf, '\n', sb->len - ignore_footer);
if (!eol)
eol = sb->buf;
/* Start of line */
p = memrchr(sb->buf, '\n', eol - sb->buf);
if (p)
p++;
else
p = sb->buf;
if (p == eol) /* Blank line? */
return 1;
/* "Signed-off-by"-style field */
while ((isalnum(*p) || *p == '-') && p < eol)
p++;
return *p == ':';
where memrchr is defined roughly as follows[1]:
#ifdef __GLIBC_PREREQ
#if __GLIBC_PREREQ(2, 2)
#define HAVE_MEMRCHR
#endif
#endif
#ifndef HAVE_MEMRCHR
#define memrchr gitmemrchr
static inline void *gitmemrchr(const void *s, int c, size_t n)
{
const unsigned char *p = s;
p += n;
while (p != s)
if (*--p == (unsigned char) c)
return p;
return NULL;
}
#endif
Does that look right?
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/159081/focus=159121
^ permalink raw reply
* Re: [PATCH v2 02/10] t/test-lib-functions.sh: allow to specify the tag name to test_commit
From: Jonathan Nieder @ 2013-01-22 8:02 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-3-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> The <message> part of test_commit() may not be appropriate for a tag name.
> So let's allow test_commit to accept a fourth argument to specify the tag
> name.
Yes!
[...]
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -135,12 +135,13 @@ test_pause () {
> fi
> }
>
> -# Call test_commit with the arguments "<message> [<file> [<contents>]]"
> +# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
> #
> # This will commit a file with the given contents and the given commit
> -# message. It will also add a tag with <message> as name.
> +# message. It will also add a tag with <message> as name unless <tag> is
> +# given.
> #
> -# Both <file> and <contents> default to <message>.
> +# <file>, <contents>, and <tag> all default to <message>.
Simpler:
# This will commit a file with the given contents and the given commit
# message and tag the resulting commit with the given tag name.
#
# <file>, <contents>, and <tag> all default to <message>.
With or without that change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [RFC] Instruct git-completion.bash that we are in test mode
From: Jean-Noël Avila @ 2013-01-22 8:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Jean-Noël AVILA, git
In-Reply-To: <7vham9dej2.fsf@alter.siamese.dyndns.org>
Le 22/01/2013 05:31, Junio C Hamano a écrit :
> Jeff King <peff@peff.net> writes:
>
>> I really hate to suggest this, but should it be more like:
>>
>> if test -z "$FAKE_COMMAND_LIST"; then __git_cmdlist() { git help -a
>> | egrep '^ [a-zA-Z0-9]' } else __git_cmdlist() { printf '%s'
>> "$FAKE_COMMAND_LIST" } fi
>>
>> That gives us a nice predictable starting point for actually
>> testing the completion code. The downside is that it doesn't let
>> us test that we remain compatible with the output of "help -a".
>
> Yeah, I think this is simpler and more to the point for the test in
> t9902. If we really want to test something that is the same as, or
> at least any closer than this approach (or my "help --standard"), to
> what the real users use, the test has to become inherently flaky, so
> I think we should go for the simplicity of this patch shows.
Instead of imposing the list of command, we could use the command
list argument to filter the ouput of git help -a. This would ensure that the
completions we want to test are still present in the installation while
still restricting them to the test case.
^ permalink raw reply
* Re: [PATCH v2 03/10] t/t3511: add some tests of 'cherry-pick -s' functionality
From: Jonathan Nieder @ 2013-01-22 8:17 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-4-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> --- /dev/null
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -0,0 +1,111 @@
[...]
> + test_commit "$mesg_one_line" foo b mesg-one-line &&
> + git reset --hard initial &&
> + test_commit "$mesg_no_footer" foo b mesg-no-footer &&
> + git reset --hard initial &&
> + test_commit "$mesg_broken_footer" foo b mesg-broken-footer &&
> + git reset --hard initial &&
> + test_commit "$mesg_with_footer" foo b mesg-with-footer &&
> + git reset --hard initial &&
> + test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob &&
Neat.
[...]
> +test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
In particular, a blank line after a one-line subject that starts with
the usual "subsystem:" convention is not mistaken for an RFC2822-style
header. Good.
[...]
> +test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
IIUC this is an illustration of false-positives from messages like
this one:
base: do something great without a sign-off
If he does that, it will be the best thing in the
world: or so I think.
A worthy cause. Could the example broken message be tweaked to
emphasize that use case? With the current example, I'd consider
either result (blank line or no blank line) to be ok behavior by git.
[...]
> +test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '
This is a simpler version of the previous test, without the distracting
colon.
[...]
> +test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '
Nice test for basic "-s" functionality.
[...]
> +test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '
And the other side of basic "-s" functionality.
One more test would be interesting: what does "-s" do when asked to
produce a duplicate signoff with an interspersed signoff by someone else?
test: a patch with a more complicated life
This patch bounced from $GIT_COMMITTER_NAME to Ms. Thor for
tweaking, then back to $GIT_COMMITTER_NAME who will be
recording it in permanent history.
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
Signed-off-by: A U Thor <author@example.com>
With the changes suggested above or similar ones,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCHv2] l10n: de.po: fix some minor issues
From: Joachim Schmitz @ 2013-01-22 8:18 UTC (permalink / raw)
To: git
In-Reply-To: <1358798856-5185-1-git-send-email-ralf.thielow@gmail.com>
Ralf Thielow wrote:
> This fixes some minor issues and improves the
> German translation a bit. The following things
> were changed:
> - use complete sentences in option related messages
> - translate "use" consistently as "verwendet"
> - don't translate "make sense" as "macht Sinn"
While your translations for these are OK, I wonder whether the original
english message needs to get fixed/changed, e.g. rather than "does't make
sense" use "are mutually exclusive" or "cannot be used with" or "cannot be
used together" like in other places?
Bye, Jojo
^ permalink raw reply
* Re: [PATCH v3] Enable minimal stat checking
From: Johannes Sixt @ 2013-01-22 8:25 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Junio C Hamano, Shawn Pearce
In-Reply-To: <1358840962-12316-1-git-send-email-robin.rosenberg@dewire.com>
Am 1/22/2013 8:49, schrieb Robin Rosenberg:
> Specifically the fields uid, gid, ctime, ino and dev are set to zero
> by JGit. Other implementations, eg. Git in cygwin are allegedly also
> somewhat incompatible with Git For Windows and on *nix platforms
> the resolution of the timestamps may differ.
>
> Any stat checking by git will then need to check content, which may
> be very slow, particularly on Windows. Since mtime and size
> is typically enough we should allow the user to tell git to avoid
> checking these fields if they are set to zero in the index.
Isn't this paragraph about slowness in the commit message misleading, as
what the patch does has no influence on the speed of stat checking? Am I
missing something?
> This change introduces a core.checkstat config option where the
> the user can select to check all fields (default), or just size
> and the whole second part of mtime (minimal).
> +core.checkstat::
> + Determines which stat fields to match between the index
> + and work tree. The user can set this to 'default' or
> + 'minimal'. Default (or explicitly 'default'), is to check
> + all fields, including the sub-second part of mtime and ctime.
I think this needs some more clarification, less 1337 speak, as well as a
hint when to set the option.
Determines which file attributes are checked to detect whether
a file has been modified. Set this option to 'minimal', when...,
which checks only the file size and whole-seconds of the last
modification time. Otherwise, leave unset or set to the value
'default'.
By starting with the hint when to set to 'minimal' in this way allows us
to omit a specification what the 'default' is.
> diff --git a/read-cache.c b/read-cache.c
> index fda78bc..23db681 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -197,21 +197,25 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
> }
> if (ce->ce_mtime.sec != (unsigned int)st->st_mtime)
> changed |= MTIME_CHANGED;
> - if (trust_ctime && ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> - changed |= CTIME_CHANGED;
> + if (trust_ctime ? check_stat : trust_ctime/*false*/)
> + if (ce->ce_ctime.sec != (unsigned int)st->st_ctime)
> + changed |= CTIME_CHANGED;
It took me a while to understand why you write /*false*/ there. Isn't the
the condition merely this:
if (trust_ctime && check_stat &&
ce->ce_ctime.sec != (unsigned int)st->st_ctime)
changed |= CTIME_CHANGED;
>
> #ifdef USE_NSEC
> - if (ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
> + if (check_stat && ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))
> changed |= MTIME_CHANGED;
> - if (trust_ctime && ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
> - changed |= CTIME_CHANGED;
> + if (trust_ctime ? check_stat : trust_ctime/*false*/)
> + if (ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))
> + changed |= CTIME_CHANGED;
Same here.
> #endif
-- Hannes
^ permalink raw reply
* Re: [PATCH v2 04/10] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
From: Jonathan Nieder @ 2013-01-22 8:27 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-5-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Let's detect "(cherry picked from...)" as part of the footer so that we
> will produce this:
>
> Signed-off-by: A U Thor <author@example.com>
> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
> Signed-off-by: C O Mmitter <committer@example.com>
>
> instead of this:
>
> Signed-off-by: A U Thor <author@example.com>
> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
>
> Signed-off-by: C O Mmitter <committer@example.com>
Yes, looks sane.
A downside is that this produces an arguably worse result when using
"-x -s" for a commit that does not already have a sign-off. Before,
we had:
test: do something great
Do something fantastic in a clean and elegant way that
only takes two lines of explanation.
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C H Errypicker <cherry-picker@example.com>
Afterwards, we will have:
test: do something great
Do something fantastic in a clean and elegant way that
only takes two lines of explanation.
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C H Errypicker <cherry-picker@example.com>
An ideal result would be completely different:
test: do something great
commit da39a3ee5e6b4b0d3255bfef95601890afd80709 upstream.
Do something fantastic in a clean and elegant way that
only takes two lines of explanation.
Signed-off-by: C H Errypicker <cherry-picker@example.com>
In other words, the -x output format that puts the commit id at the
end with odd spacing seems to be of questionable taste anyway. But
given the constraint of leaving that alone, cramming together the
sign-off like this seems like the best we can do, so for what it's
worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 05/10] sequencer.c: always separate "(cherry picked from" from commit body
From: Jonathan Nieder @ 2013-01-22 8:32 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-6-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Start treating the "(cherry picked from" line added by cherry-pick -x
> the same way that the s-o-b lines are treated. Namely, separate them
> from the main commit message body with an empty line.
Heh, you read my mind.
^ permalink raw reply
* Re: [PATCH 3/7] contrib/subtree: Add --unannotate
From: greened @ 2013-01-22 8:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, James Nylen
In-Reply-To: <7vy5ftzr3s.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> greened@obbligato.org writes:
>
>> greened@obbligato.org writes:
>>
>>>> I think this paragraph inherits existing breakage from the beginning
>>>> of time, but I do not think the above will format the second and
>>>> subsequent paragraphs correctly.
>>>
>>> Ok, I'll take a look.
>>
>> I don't know what "correctly" is but it is at least formatted in a
>> similar manner to the other options.
>
> That is what "inherits existing breakage" means ;-)
Ah.
> The first paragraph is typeset as body text, while the rest are
> typeset in monospaced font, no?
>
> It should be more like this, I think:
>
> --option::
> First paragraph
> +
> Second paragraph
> +
> And third paragraph
Ok.
-David
^ permalink raw reply
* Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b
From: Jonathan Nieder @ 2013-01-22 8:38 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <1358757627-16682-7-git-send-email-drafnel@gmail.com>
Brandon Casey wrote:
> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.
> This is in preparation to unify the append_signoff implementations in
> log-tree.c and sequencer.c.
[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1082,9 +1101,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
> return pick_commits(todo_list, opts);
> }
>
> -void append_signoff(struct strbuf *msgbuf, int ignore_footer)
> +void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
Isn't the behavior of passing '1' here just a bug in "format-patch -s"?
Style: callers will be easier to read if the function takes a flag
word with named bits, as in:
#define APPEND_SIGNOFF_DEDUP (1 << 0)
void append_signoff(..., int ignore_footer, unsigned flag)
^ permalink raw reply
* Re: [PATCH 2/7] contrib/subtree: Use %B for Split Subject/Body
From: greened @ 2013-01-22 8:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: 郑文辉(Techlive Zheng), git
In-Reply-To: <7va9s9yvzo.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> greened@obbligato.org writes:
>
>> Are you incorporating the other patches? Should I drop them
>> from my list?
>
> I actually was planning to accept patches to this subdirectory only
> through you, hopefully as messages that forward others' changes with
> your Acked-by: tagline. That frees me from having to keeping track
> of what goes on there ;-)
Ok, just wanted to know the process. Makes sense to me.
-David
^ permalink raw reply
* Re: [PATCH] Add --unannotate option to git-subtree
From: greened @ 2013-01-22 8:41 UTC (permalink / raw)
To: James Nylen; +Cc: git
In-Reply-To: <CABVa4NhwcD584ptSazOR9WvSWep1z+krhxkWDvUk8nXaF8EYxQ@mail.gmail.com>
James Nylen <jnylen@gmail.com> writes:
> Wow, I missed a bunch of emails on this. Thanks for applying and for
> writing tests!
Sorry it took so long.
> This is as intended. You wouldn't want subtree to modify commits that
> occurred in the full repository for project A. Furthermore, you
> wouldn't have a "subproj:" commit in project A's standalone repo since
> it wasn't a subproject at that time.
Yes, that makes sense.
> The --annotate option confused me because it was the reverse of what I
> wanted. As in your example, a typical use would be 'add a file to
> subdir with message "subproj: add F3" ' to make it clear that you were
> committing to the "subproj" part of a larger repository. Then, when
> splitting back out to subproj's main repository, you'd want to remove
> the prefix.
Ok. I'll re-submit as part of the final sequence.
Thanks for the patch!
-David
^ permalink raw reply
* Re: [PATCH 2/8] Add --unannotate
From: greened @ 2013-01-22 8:44 UTC (permalink / raw)
To: James Nylen; +Cc: Junio C Hamano, git
In-Reply-To: <CABVa4NhK3FR-NsTq6Vt6yrgneQmMxF5ANmN6pF8k3fHeOLd0JA@mail.gmail.com>
James Nylen <jnylen@gmail.com> writes:
> I just now saw these emails. I'm having a hard time thinking of any
> good use case other than:
>
> - add "fancylib" as a subtree of "myprog"
> - commit to myprog repo: "fancylib: don't crash as much"
> - split these commits back out to fancylib's main repo, and remove
> the "fancylib: " prefix
That does seem to me to be the common case, at least.
> You could potentially have something like "Don't crash as much
> (fancylib)" but that's awkward. What might you want to do with a
> pattern-based rewrite that doesn't involve removing a prefix when
> splitting commits?
I'm not really sure. I've never used --annotate in my own work.
> In fact, I don't see the use of the original --annotate option at all,
> since it causes more detailed commit messages in the smaller of the
> two repositories.
I'll have to look back through Avery's logs and see if I can puzzle out
why this was added. If it's not useful, perhaps we can remove it before
migrating to mainline.
Junio, is there a policy for backward-compatability in contrib? I hope
that since that directory is for stuff not yet in mainline, there is
some room to massage the user interface.
-David
^ permalink raw reply
* Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer
From: Brandon Casey @ 2013-01-22 9:35 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <20130122075413.GB6085@elie.Belkin>
On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Brandon Casey wrote:
>
>> --- a/sequencer.c
>> +++ b/sequencer.c
> [...]
>> @@ -1042,13 +1041,8 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
>
> Git is checking if (sb->buf) ends with a "Signed-off-by:" style
> line. If it doesn't, it will need to add an extra blank line
> before adding a new sign-off.
>
> First (snipped), it seeks back two newlines from the end and then
> forward to the next non-newline character, so (buf + i) is at the
> start of the last line of (the interesting part of) sb.
Did you catch that the two newlines have to be adjacent to each other?
i.e. it seeks back until it finds a blank line. Then it seeks
forward so that buf + i points at the first line of the last paragraph
of sb.
> Now:
>
>> for (; i < len; i = k) {
>> for (k = i; k < len && buf[k] != '\n'; k++)
>> ; /* do nothing */
>> k++;
>
> (buf + k) points to the end of this line.
buf + k points to the start of the next line. Not sure if that is the
same thing as what you said.
>> - if ((buf[k] == ' ' || buf[k] == '\t') && !first)
>> - continue;
>
> This is always the first line examined, so this "continue" never
> triggers.
This is just totally broken and always has been. The index variable
should be 'i' not 'k'. It is supposed to check whether the current
line is a continuation of the previously inspected line. An rfc2822
continuation line begins with space or tab. The first line of the
paragraph obviously can't be a continuation line, so the /first/
variable is used as a guard against matching the first line.
>> -
>> - first = 0;
>> -
>> for (j = 0; i + j < len; j++) {
>
> If the line matches /^[[:alnum:]-]*:/, it passes and git moves on to
> the (nonexistent) next line. Otherwise, it fails.
>
> Do I understand correctly?
No, I think you misread the for loop that you snipped out. It seeks
back to the beginning of the last paragraph, not the last line. The
for loop that you included above, inspects each line of that
paragraph. If any line does not match /^[[:alnum:]-]*:/, then the
function returns false (0). If every line matches, it returns true
(1).
> If so, this patch should be a no-op, which
> is good, I guess.
You're correct here though. The patch is a no-op. This patch removes
the code that was supposed to parse rfc2822 continuation lines, but it
was broken. Thankfully it was broken utterly and completely so it
never allowed anything through that wasn't /^[[:alnum:]-]*:/.
-Brandon
^ permalink raw reply
* Re: [PATCH v2 02/10] t/test-lib-functions.sh: allow to specify the tag name to test_commit
From: Brandon Casey @ 2013-01-22 9:43 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <20130122080216.GC6085@elie.Belkin>
On Tue, Jan 22, 2013 at 12:02 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -135,12 +135,13 @@ test_pause () {
>> fi
>> }
>>
>> -# Call test_commit with the arguments "<message> [<file> [<contents>]]"
>> +# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
>> #
>> # This will commit a file with the given contents and the given commit
>> -# message. It will also add a tag with <message> as name.
>> +# message. It will also add a tag with <message> as name unless <tag> is
>> +# given.
>> #
>> -# Both <file> and <contents> default to <message>.
>> +# <file>, <contents>, and <tag> all default to <message>.
>
> Simpler:
>
> # This will commit a file with the given contents and the given commit
> # message and tag the resulting commit with the given tag name.
> #
> # <file>, <contents>, and <tag> all default to <message>.
Yes, a nice improvement.
Thanks,
-Brandon
^ permalink raw reply
* Aw: Re: [PATCH v3 1/6] Change old system name 'GIT' to 'Git'
From: Thomas Ackermann @ 2013-01-22 9:44 UTC (permalink / raw)
To: gitster, th.acker; +Cc: davvid, git
In-Reply-To: <7vfw1udpav.fsf@alter.siamese.dyndns.org>
>
> I think it misses "GIT - the stupid content tracker" in README, but
> probably it is OK (it is not an end-user facing documentation).
>
I only checked ./Documentation; but this should be changed also.
> I noticed that these two places still use poor-man's small caps
> after this patch.
>
Thanks.
> * Documentation/SubmittingPatches:
> that are being emailed around. Although core GIT is a lot
>
I will change this.
> * Documentation/git-credential.txt:
> TYPICAL USE OF GIT CREDENTIAL
>
This is to be debated because here all caps is used as "poor man's bold face"
and it would look odd if we write 'TYPICAL USE OF Git CREDENTIAL'?
>
> Not commenting on other 5 patches in the series yet, but if they
> interact with other topics in flight, they may have to be separated
> out. We'll see.
>
Perhaps it would be best to separate this into 2 series:
[1/6]+[6/6] and [2/6]-[5/6]?
---
Thomas
^ permalink raw reply
* Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer
From: Jonathan Nieder @ 2013-01-22 9:47 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <CA+sFfMfERgGbf58LjOfAhhO_-YLu+yo+L-vYMuAuOUaP90yvgA@mail.gmail.com>
Brandon Casey wrote:
> On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> First (snipped), it seeks back two newlines from the end and then
>> forward to the next non-newline character, so (buf + i) is at the
>> start of the last line of (the interesting part of) sb.
>
> Did you catch that the two newlines have to be adjacent to each other?
[...]
Here is the loop in master:
int hit = 0;
[...]
for (i = len - 1; i > 0; i--) {
if (hit && buf[i] == '\n')
break;
hit = (buf[i] == '\n');
}
I don't see any adjacency check. I agree with you that "two adjacent
newlines" was probably the intent, though.
^ permalink raw reply
* Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer
From: Jonathan Nieder @ 2013-01-22 9:49 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <20130122094720.GA8908@elie.Belkin>
Jonathan Nieder wrote:
> Here is the loop in master:
>
> int hit = 0;
> [...]
>
> for (i = len - 1; i > 0; i--) {
> if (hit && buf[i] == '\n')
> break;
> hit = (buf[i] == '\n');
> }
>
> I don't see any adjacency check.
Of course that's because I can't read. :) Checking again.
^ permalink raw reply
* Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer
From: Jonathan Nieder @ 2013-01-22 10:12 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <CA+sFfMfERgGbf58LjOfAhhO_-YLu+yo+L-vYMuAuOUaP90yvgA@mail.gmail.com>
Brandon Casey wrote:
> On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> - if ((buf[k] == ' ' || buf[k] == '\t') && !first)
>>> - continue;
>>
>> This is always the first line examined, so this "continue" never
>> triggers.
>
> This is just totally broken and always has been. The index variable
> should be 'i' not 'k'.
Yes, now I see.
This test trips when the *next* line starts with ' ' or '\t'.
commit, cherry-pick -s: remove broken support for multiline rfc2822 fields
Starting with c1e01b0c (commit: More generous accepting of RFC-2822
footer lines, 2009-10-28), "git commit -s" carefully parses the last
paragraph of each commit message to check if it consists only of
RFC2822-style headers, in which case the signoff will be added as a
new line in the same list:
Reported-by: Reporter <reporter@example.com>
Signed-off-by: Author <author@example.com>
Acked-by: Lieutenant <lt@example.com>
It even included support for accepting indented continuation lines for
multiline fields. Unfortunately the multiline field support is broken
because it checks whether buf[k] (the first character of the *next*
line) instead of buf[i] is a whitespace character. So for example, it
confuses the following for a well-formed RFC2822 footer:
Reported-by: Reporter <reporter@example.com>
LINE NOISE
continuation
continuation
A typical well-formed footer with multiline fields is not accepted:
Reported-by: Re Porter
<reporter@example.com>
Signed-off-by: Author <author@example.com>
That this has remained broken for so long is a good sign that nobody
actually needed continuation lines. Rip out the broken continuation
support.
^ permalink raw reply
* Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer
From: Jonathan Nieder @ 2013-01-22 10:21 UTC (permalink / raw)
To: Brandon Casey; +Cc: gitster, pclouds, git, Brandon Casey
In-Reply-To: <20130122101208.GC8908@elie.Belkin>
Jonathan Nieder wrote:
> line) instead of buf[i] is a whitespace character. So for example, it
> confuses the following for a well-formed RFC2822 footer:
Luckily it doesn't, since the final continuation line is not followed
by whitespace. I should have said:
"... is a whitespace character. The result is that any
footer with a continuation line is not accepted, since the last
continuation line neither starts with an RFC2822 field name nor is
followed by a continuation line.
That this has remained broken for so long is good evidence that nobody
actually needed multiline fields. Rip out the broken continuation
support.
No functional change intended."
I have no excuse, since you explained this all out loud to me a couple of
months ago. :) Sorry for the nonsense.
Thanks,
Jonathan
^ 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