All of lore.kernel.org
 help / color / mirror / Atom feed
From: Justin Frankel <justin@cockos.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Bert Wesarg <bert.wesarg@googlemail.com>,
	Jonathan Nieder <jrnieder@gmail.com>,
	git@vger.kernel.org, eyvind.bernhardsen@gmail.com
Subject: Re: [PATCH v2] git-merge: ignore space support
Date: Wed, 25 Aug 2010 11:21:02 -0700	[thread overview]
Message-ID: <4C755F0E.5080705@cockos.com> (raw)
In-Reply-To: <7vy6bushjx.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Justin Frankel <justin@cockos.com> writes:
>
>   
>> The only danger is that ll_merge()'s signature didn't change in such a
>> way to break compilation, i.e:
>>
>> int ll_merge(mmbuffer_t *result_buf,
>>             const char *path,
>>             mmfile_t *ancestor, const char *ancestor_label,
>>             mmfile_t *ours, const char *our_label,
>>             mmfile_t *theirs, const char *their_label,
>>             int flag);
>>
>> becomes:
>>
>> int ll_merge(mmbuffer_t *result_buf,
>>             const char *path,
>>             mmfile_t *ancestor, const char *ancestor_label,
>>             mmfile_t *ours, const char *our_label,
>>             mmfile_t *theirs, const char *their_label,
>>             struct whatever *conf);
>>
>> In this case, passing 0 as the last parameter will compile either way.
>>
>> Sure, we can grep all of the source, but who knows when something else
>> will get merged in...
>>     
>
> That is technically a valid concern but I suspect it does not matter in
> this particular case, where integer 0 used to mean "use the default" and
> the new API uses NULL to mean the same.
>
> If an existing call site used to pass 0 and the patch forgot to update it,
> it will look ugly (we encourage to spell a NULL pointer "NULL", not "0",
> in our codebase) but no harm is done.  If an existing call site asked for
> a non-default behaviour by passing a non-zero integer flag, and the patch
> forgot to update it, the compiler would have caught it.  Merging a side
> branch is the same deal; if it adds a call with a non-zero argument to ask
> for a non-default behaviour, that will be done via an expression over some
> integer variables or constants, and that won't be casted silently to a
> pointer to "struct whatever", no?
>
>   
Agreed, I was responding to Bert's email, in which he stated that he 
hadn't seen NULL-for-default anywhere else in git. Using NULL for 
default behavior is good in that it handles un-updated code and merges 
correctly (passing 0 uses defaults, passing nonzero fails compile).


> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

  reply	other threads:[~2010-08-25 18:21 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-08-23 20:59 [PATCH v2] git-merge: ignore space support Justin Frankel
2010-08-24  2:28 ` Jonathan Nieder
2010-08-24  3:39   ` [RFC/PATCH jn/merge-renormalize] merge-recursive: expose merge options for builtin merge Jonathan Nieder
2010-08-24 18:52     ` Junio C Hamano
2010-08-25  4:29       ` Jonathan Nieder
2010-08-24  4:30   ` [PATCH v2] git-merge: ignore space support Justin Frankel
2010-08-25  4:40     ` Jonathan Nieder
2010-08-25  7:22       ` Bert Wesarg
2010-08-25 15:51         ` Justin Frankel
2010-08-25 17:55           ` Junio C Hamano
2010-08-25 18:21             ` Justin Frankel [this message]
2010-08-24 19:01   ` Junio C Hamano
2010-08-24 20:01   ` Bert Wesarg
2010-08-25  3:57     ` Jonathan Nieder
2010-08-26  5:41 ` [PATCH v3 0/4] " Jonathan Nieder
2010-08-26  5:47   ` [PATCH 1/4] merge-recursive: expose merge options for builtin merge Jonathan Nieder
2010-08-26  5:49   ` [PATCH 2/4] ll-merge: replace flag argument with options struct Jonathan Nieder
2010-08-26 16:39     ` Junio C Hamano
2011-01-16  1:08       ` [PATCH v1.7.4-rc2] ll-merge: simplify opts == NULL case Jonathan Nieder
2010-08-26  5:50   ` [PATCH 3/4] merge-recursive --patience Jonathan Nieder
2010-08-26  5:51   ` [PATCH 4/4] merge-recursive: options to ignore whitespace changes Jonathan Nieder
2010-08-26 16:39     ` Junio C Hamano
2010-08-27  8:24       ` Jonathan Nieder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4C755F0E.5080705@cockos.com \
    --to=justin@cockos.com \
    --cc=bert.wesarg@googlemail.com \
    --cc=eyvind.bernhardsen@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.