Re: [PATCH] replace: parse revision argument for -d
Jeff King venit, vidit, dixit 29.10.2012 10:04: > On Mon, Oct 29, 2012 at 10:02:47AM +0100, Michael J Gruber wrote: > >> Jeff King venit, vidit, dixit 29.10.2012 07:58: >>> On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote: >>> for (p = argv; *p; p++) { - if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p) + q = *p; + if (get_sha1(q, sha1)) + warning("Failed to resolve '%s' as a valid ref; taking it literally.", q); + else + q = sha1_to_hex(sha1); >>> >>> Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything >>> else, it would not work anyway)? >> >> What is "this"??? >> >> So far, "git replace -d " only accepts a full sha1, because it uses >> it literally as a ref name "resf/replace/" without resolving anything. >> >> The patch makes it so that gets resolved to a sha1 even if it is >> abbreviated, and then it gets used. >> >> Or do you mean the warning? > > Sorry, yeah, I meant the warning and fallback. > > If I understand correctly, the fallback will never work unless we are > fed a 40-byte sha1. But get_sha1 should always return a 40-byte sha1 > without doing any further processing. You do understand correctly, and I misunderstood get_sha1(). I does not check for the presence of that sha1 in the object db, so that it succeeds no matter what, that is: if it's valid hex. So I should probably rewrite the error handling: no more need to check for lengths. Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] replace: parse revision argument for -d
On Mon, Oct 29, 2012 at 10:02:47AM +0100, Michael J Gruber wrote: > Jeff King venit, vidit, dixit 29.10.2012 07:58: > > On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote: > > > >>for (p = argv; *p; p++) { > >> - if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p) > >> + q = *p; > >> + if (get_sha1(q, sha1)) > >> + warning("Failed to resolve '%s' as a valid ref; taking > >> it literally.", q); > >> + else > >> + q = sha1_to_hex(sha1); > > > > Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything > > else, it would not work anyway)? > > What is "this"??? > > So far, "git replace -d " only accepts a full sha1, because it uses > it literally as a ref name "resf/replace/" without resolving anything. > > The patch makes it so that gets resolved to a sha1 even if it is > abbreviated, and then it gets used. > > Or do you mean the warning? Sorry, yeah, I meant the warning and fallback. If I understand correctly, the fallback will never work unless we are fed a 40-byte sha1. But get_sha1 should always return a 40-byte sha1 without doing any further processing. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] replace: parse revision argument for -d
Jeff King venit, vidit, dixit 29.10.2012 07:58: > On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote: > >> for (p = argv; *p; p++) { >> -if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p) >> +q = *p; >> +if (get_sha1(q, sha1)) >> +warning("Failed to resolve '%s' as a valid ref; taking >> it literally.", q); >> +else >> +q = sha1_to_hex(sha1); > > Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything > else, it would not work anyway)? What is "this"??? So far, "git replace -d " only accepts a full sha1, because it uses it literally as a ref name "resf/replace/" without resolving anything. The patch makes it so that gets resolved to a sha1 even if it is abbreviated, and then it gets used. Or do you mean the warning? Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] replace: parse revision argument for -d
On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote: > for (p = argv; *p; p++) { > - if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p) > + q = *p; > + if (get_sha1(q, sha1)) > + warning("Failed to resolve '%s' as a valid ref; taking > it literally.", q); > + else > + q = sha1_to_hex(sha1); Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything else, it would not work anyway)? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] replace: parse revision argument for -d
Hi, On Fri, Oct 26, 2012 at 3:33 PM, Michael J Gruber wrote: > 'git replace' parses the revision arguments when it creates replacements > (so that a sha1 can be abbreviated, e.g.) but not when deleting > replacements. > > This sucks. > > Make it parse the argument to 'replace -d' in the same way. Nit: there could be more than one argument to 'replace -d', so perhaps "each argument" is better. > Just in case someone lost the replacement object before deleting the > replacement, take the argument literally if it can not be resolved to a Here too. > full sha1. > > Signed-off-by: Michael J Gruber > --- > builtin/replace.c | 15 ++- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/builtin/replace.c b/builtin/replace.c > index e3aaf70..80e2039 100644 > --- a/builtin/replace.c > +++ b/builtin/replace.c > @@ -46,24 +46,29 @@ typedef int (*each_replace_name_fn)(const char *name, > const char *ref, > > static int for_each_replace_name(const char **argv, each_replace_name_fn fn) > { > - const char **p; > + const char **p, *q; > char ref[PATH_MAX]; > int had_error = 0; > unsigned char sha1[20]; > > for (p = argv; *p; p++) { > - if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p) > + q = *p; > + if (get_sha1(q, sha1)) > + warning("Failed to resolve '%s' as a valid ref; > taking it literally.", q); > + else > + q = sha1_to_hex(sha1); > + if (snprintf(ref, sizeof(ref), "refs/replace/%s", q) > >= sizeof(ref)) { > - error("replace ref name too long: %.*s...", 50, *p); > + error("replace ref name too long: %.*s...", 50, q); > had_error = 1; > continue; > } > if (read_ref(ref, sha1)) { > - error("replace ref '%s' not found.", *p); > + error("replace ref '%s' not found.", q); > had_error = 1; > continue; > } > - if (fn(*p, ref, sha1)) > + if (fn(q, ref, sha1)) > had_error = 1; > } > return had_error; Looks good to me. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] replace: parse revision argument for -d
'git replace' parses the revision arguments when it creates replacements (so that a sha1 can be abbreviated, e.g.) but not when deleting replacements. This sucks. Make it parse the argument to 'replace -d' in the same way. Just in case someone lost the replacement object before deleting the replacement, take the argument literally if it can not be resolved to a full sha1. Signed-off-by: Michael J Gruber --- builtin/replace.c | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index e3aaf70..80e2039 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -46,24 +46,29 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref, static int for_each_replace_name(const char **argv, each_replace_name_fn fn) { - const char **p; + const char **p, *q; char ref[PATH_MAX]; int had_error = 0; unsigned char sha1[20]; for (p = argv; *p; p++) { - if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p) + q = *p; + if (get_sha1(q, sha1)) + warning("Failed to resolve '%s' as a valid ref; taking it literally.", q); + else + q = sha1_to_hex(sha1); + if (snprintf(ref, sizeof(ref), "refs/replace/%s", q) >= sizeof(ref)) { - error("replace ref name too long: %.*s...", 50, *p); + error("replace ref name too long: %.*s...", 50, q); had_error = 1; continue; } if (read_ref(ref, sha1)) { - error("replace ref '%s' not found.", *p); + error("replace ref '%s' not found.", q); had_error = 1; continue; } - if (fn(*p, ref, sha1)) + if (fn(q, ref, sha1)) had_error = 1; } return had_error; -- 1.8.0.370.g8cbad08 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html