On Mon, 30 Aug 2010, Junio C Hamano wrote:
Thanks for the critique and comments
> Dave Olszewski <cxreg@xxxxxxxxx> writes:
>
> > Generally, tags are considered a write-once ref (or object), and updates
> > to them are the exception to the rule.
>
> This may be just the naming issue and you could say "moving them",
> "updates to them" or "changing them" interchangeably in the above;
> among them, "updates to them" sounds the most natural.
>
> Can you change the "moving" in the patch to make them consistent with the
> above description?
Sure, no problem. Would you like this changed in the variable and
config names as well, or just the printed text?
> > diff --git a/remote.c b/remote.c
> > index 9143ec7..fbca1e6 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -50,6 +50,8 @@ static int explicit_default_remote_name;
> > static struct rewrites rewrites;
> > static struct rewrites rewrites_push;
> >
> > +static int deny_moving_tags;
> > +
> > #define BUF_SIZE (2048)
> > static char buffer[BUF_SIZE];
> >
> > @@ -385,6 +387,10 @@ static int handle_config(const char *key, const char *value, void *cb)
> > add_instead_of(rewrite, xstrdup(value));
> > }
> > }
> > + if (!strcmp(key, "push.denymovingtags")) {
> > + deny_moving_tags = git_config_bool(key, value);
> > + return 0;
> > + }
>
> Hmm, shouldn't this be per-remote (rather, shouldn't a per-remote variant
> be allowed to override this)?
I wasn't sure about this. I like the idea of a single setting with
per-remote override, I'll implement that.
> > @@ -1266,6 +1272,31 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
> > continue;
> > }
> >
> > + /* If a tag already exists on the remote and points to
> > + * a different object, we don't want to push it again
> > + * without requiring the user to indicate that they know
> > + * what they are doing.
> > + */
>
> /*
> * We try to format
> * multi-line comment
> * like this.
> */
Ok.
> > + if (!prefixcmp(ref->name, "refs/tags/") &&
> > + !ref->deletion &&
> > + !is_null_sha1(ref->old_sha1)) {
> > + if (deny_moving_tags) {
> > + /* Set `nonfastforward` for the sake of displaying
> > + * this update as forced
> > + */
> > + ref->nonfastforward = 1;
>
> I think you are propagating this bit to print_ok_ref_status() in
> transport.c; it indicates that after your change, "nonfastforward" does
> not mean non-fast-forward anymore, doesn't it?
>
> Perhaps the bit needs to be renamed to "update_forced" or something?
Good point. I arrived at making this change pretty late in the patch
and didn't consider the rename. Thanks.
> > + if (!ref->force && !force_update) {
> > + ref->status = REF_STATUS_REJECT_MOVING_TAG;
> > + }
> > + } else {
> > + if (!ref->force && !force_update)
> > + warning("You are changing the value of an upstream tag. This may\n"
> > + "be deprecated in a future version of Git. Please use --force\n"
> > + "if this was intentional, and consider setting push.denyMovingTags.");
> > + }
> > + continue;
> > + }
> > +
> > /* This part determines what can overwrite what.
> > * The rules are:
> > *
>
> You are changing the rule that determine what can overwrite what, aren't
> you? It is Ok (although it is in general frowned upon if you do so when
> you do not have to) to add your new rule before an existing rule, but your
> rule should be added as a new rule to the enumeration in the comment, and
> the code that implements the new rule after the comment, no?
The reason I wanted to put it first is that a tag update could be either
fast-forward or not, and I wanted to have consistent behavior for both
cases. I can move the comment block and describe the full set of cases.
> > diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> > index c718253..7906ba5 100755
> > --- a/t/t5400-send-pack.sh
> > +++ b/t/t5400-send-pack.sh
> > @@ -106,6 +106,20 @@ test_expect_success 'denyNonFastforwards trumps --force' '
> > test "$victim_orig" = "$victim_head"
> > '
> >
> > +test_expect_success 'denyMovingTags trumps --force' '
> > + (
> > + cd victim &&
> > + ( git tag moving_tag master^ || : ) &&
>
> In which circumstance is it allowed for this "git tag" command to
> fail and the entire test to succeed?
Cargo-cult error, good catch, thanks.
Fixed patch forthcoming.
Dave
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html